Hi Cupertino,
Thanks for the patch!
Some comments inline below.
On 1/21/26 13:25, Cupertino Miranda wrote:
> This patch corrects CO-RE generation for the cases where an expression
> starts with a non-CO-RE access, but in the middle it requires to
> generate CO-RE to correctly compute the access location.
>
> It fixes it by splitting the expression into their CO-RE and non-CO-RE
> counterparts. It performs this by walking gimple expressions, and for
> each field access to which its type is a struct or union, it verifies if
> both the types for the base and field are attributed similarly.
> Otherwise, it splits the expression at this location by creating a
> temporary variable and performing any required pointer conversions.
> This smaller expressions are converted to CO-RE in the subsequent
> gimple walker.
>
> There is no way in GCC to distinguish nested struct/union definitions
> from non-nested ones.
> This patch simplifies code and enforces that all preserve_access_index
> structs/unions would be attributed explicity.
typo: explicitly
> Previous approach was error prone as it would extend CO-RE accesses to
> structures which would not be attributed.
>
> All GCC BPF dejagnu passes tests:
> # of expected passes 553
> # of expected failures 6
>
> kernel-next bpf selftests:
> Summary: 528/4794 PASSED, 113 SKIPPED, 151 FAILED
Hm, do you have a delta for before/after this patch for these numbers?
It is difficult to compare absolute numbers (until we're at 0 fails)
without knowing exactly the which commit used on which kernel branch.
Just want to know whether this patch makes it better.
(or if it reveals another underlying issue, that is good to know also)
>
> BPF selftests were execyted with the following result:
Was something cut off here, or this line goes with the above?
Also: execyted -> executed
>
> gcc/ChangeLog:
> PR target/120241
> * config/bpf/core-builtins.cc
> (is_attr_preserve_access): Correct for pointer types.
> (maybe_get_base_for_field_expr, core_access_index_map,
> core_access_clean, core_is_access_index, core_mark_as_access_index):
> Remove.
> (make_gimple_core_safe_access_index): Remove call to
> core_is_access_index.
> (callback_find_next_split_location, core_should_split_expr,
> find_next_split_location, gimple_core_early_split_expr): Add
> function to split expressions in CO-RE and non-CO-RE
> expressions.
> (execute_lower_bpf_core): Adapt to new code.
>
> gcc/testsuite/ChangeLog:
I think a PR target/120241 belongs here as well.
And in the commit header/subject line as [PR120241] for that matter,
so that once pushed it will be linked to the PR.
> * gcc.target/bpf/core-attr-3.c: Add attribute.
> * gcc.target/bpf/core-attr-4.c: Add attribute.
> * gcc.target/bpf/core-attr-5.c: Add attribute.
> * gcc.target/bpf/core-attr-6.c: Add attribute.
> * gcc.target/bpf/core-attr-7.c: New test.
> * gcc.target/bpf/core-attr-calls.c: Adapt.
> ---
> gcc/config/bpf/core-builtins.cc | 187 +++++++++++-------
> gcc/gimplify.cc | 2 +-
Also noted below but looks like some spurious white space change
in gimplify.cc that should not be included.
> gcc/testsuite/gcc.target/bpf/core-attr-3.c | 4 +-
> gcc/testsuite/gcc.target/bpf/core-attr-4.c | 4 +-
> gcc/testsuite/gcc.target/bpf/core-attr-5.c | 2 +-
> gcc/testsuite/gcc.target/bpf/core-attr-6.c | 4 +-
> gcc/testsuite/gcc.target/bpf/core-attr-7.c | 150 ++++++++++++++
> .../gcc.target/bpf/core-attr-calls.c | 4 +-
> 8 files changed, 273 insertions(+), 84 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/bpf/core-attr-7.c
>
> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
> index 152da94a9761..16ee7f453bab 100644
> --- a/gcc/config/bpf/core-builtins.cc
> +++ b/gcc/config/bpf/core-builtins.cc
> @@ -324,8 +324,13 @@ is_attr_preserve_access (tree t)
> TYPE_ATTRIBUTES (TREE_TYPE (base)));
>
> if (TREE_CODE (base) == MEM_REF)
> - return lookup_attribute ("preserve_access_index",
> - TYPE_ATTRIBUTES (TREE_TYPE (base)));
> + {
> + tree type = TREE_TYPE (base);
> + if (POINTER_TYPE_P (type))
> + type = TREE_TYPE (type);
> + return lookup_attribute ("preserve_access_index",
> + TYPE_ATTRIBUTES (type));
> + }
>
> if (TREE_CODE (t) == COMPONENT_REF)
> {
> @@ -1710,68 +1715,6 @@ bpf_output_core_reloc (rtx *operands, int nr_ops)
> }
> }
>
> -static tree
> -maybe_get_base_for_field_expr (tree expr)
> -{
> - poly_int64 bitsize, bitpos;
> - tree var_off;
> - machine_mode mode;
> - int sign, reverse, vol;
> -
> - if (expr == NULL_TREE)
> - return NULL_TREE;
> -
> - return get_inner_reference (expr, &bitsize, &bitpos, &var_off, &mode,
> - &sign, &reverse, &vol);
> -}
> -
> -/* Access functions to mark sub expressions as attributed with
> - __preserve_access_index.
> - This is required since in gimple format, in order to convert an
> expression as
> - CO-RE safe, we must create multiple gimple statements.
> - Also, only the type of the base of the expression might be attributed with
> - __preserve_access_index. Nevertheless all the consecutive accesses to
> this
> - attributed node should also be converted to CO-RE safe.
> - Any LHS assigned values with CO-RE converted expressions are marked and
> - any uses of these values are later checked for further convertion.
> - The core_access_index_map functions allow to mark this nodes for later
> - convertion to CO-RE.
> - This mechanism are used by make_gimple_core_safe_access_index. */
> -
> -static GTY(()) hash_map<tree, tree> *core_access_index_map = NULL;
> -
> -static void
> -core_access_clean (void)
> -{
> - if (core_access_index_map == NULL)
> - core_access_index_map = hash_map<tree, tree>::create_ggc (10);
> - core_access_index_map->empty ();
> -}
> -
> -static bool
> -core_is_access_index (tree expr)
> -{
> - if (TREE_CODE (expr) == MEM_REF
> - || TREE_CODE (expr) == INDIRECT_REF)
> - expr = TREE_OPERAND (expr, 0);
> -
> - tree *def = core_access_index_map->get (expr);
> - if (def)
> - return true;
> - return false;
> -}
> -
> -static void
> -core_mark_as_access_index (tree expr)
> -{
> - if (TREE_CODE (expr) == MEM_REF
> - || TREE_CODE (expr) == INDIRECT_REF)
> - expr = TREE_OPERAND (expr, 0);
> -
> - if (core_access_index_map->get (expr) == NULL)
> - core_access_index_map->put (expr, NULL_TREE);
> -}
> -
> /* This function is an adaptation of make_core_safe_access_index but to be
> used
> in gimple format trees. It is used by execute_lower_bpf_core, when
> traversing the gimple tree looking for nodes that would have its type
> @@ -1792,8 +1735,7 @@ make_gimple_core_safe_access_index (tree *tp,
> patch = &(TREE_OPERAND (*tp, 0));
> tree orig_type = TREE_TYPE (*patch);
>
> - if ((is_attr_preserve_access (*patch)
> - || core_is_access_index (maybe_get_base_for_field_expr (*patch)))
> + if (is_attr_preserve_access (*patch)
> && (n = compute_field_expr (*patch, NULL, &valid, NULL)) > 0
> && valid == true)
> {
> @@ -1822,13 +1764,109 @@ make_gimple_core_safe_access_index (tree *tp,
>
> wi->changed = true;
> *walk_subtrees = false;
> + }
> + return NULL_TREE;
> +}
>
> - tree lhs;
> - if (!wi->is_lhs
> - && gimple_code (wi->stmt) != GIMPLE_CALL
> - && (lhs = gimple_get_lhs (wi->stmt)) != NULL_TREE)
> - core_mark_as_access_index (lhs);
> +static bool
> +core_should_split_expr (tree t)
> +{
> + tree type = TREE_TYPE (t);
> + if (TREE_CODE (type) != RECORD_TYPE
> + && TREE_CODE (type) != UNION_TYPE)
> + return false;
> +
> + if (TREE_CODE (t) == COMPONENT_REF)
> + {
> + tree base = TREE_OPERAND (t, 0);
> +
> + bool base_is_attr = lookup_attribute ("preserve_access_index",
> + TYPE_ATTRIBUTES (TREE_TYPE (base)));
> + bool cur_is_attr = lookup_attribute ("preserve_access_index",
> + TYPE_ATTRIBUTES (TREE_TYPE (t)));
> +
> + if (base_is_attr != cur_is_attr)
> + return true;
> }
> + return false;
> +}
> +
> +static tree
> +callback_find_next_split_location (tree *tp,
> + int *walk_subtrees ATTRIBUTE_UNUSED,
> + void *data)
> +{
> + tree **core_base_pointer = (tree **) data;
> +
> + bool should_split = core_should_split_expr (*tp);
> +
> + if (should_split == true)
Nit, but why not just:
if (core_should_split_expr (*tp))
> + {
> + *core_base_pointer = tp;
> + return *tp;
> + }
> + else
> + /* Keep walking the expression. */
> + return NULL_TREE;
> +}
> +
> +static tree *
> +find_next_split_location (tree *tp)
> +{
> +
> + tree *core_base_pointer = NULL;
> + walk_tree (tp, callback_find_next_split_location, &core_base_pointer,
> NULL);
> + return core_base_pointer;
> +}
> +
> +static tree
> +gimple_core_early_split_expr (tree *tp,
> + int *walk_subtrees,
> + void *data)
Please add a short comment here explaining briefly the transformation
done and why it is needed. If we need to revisit this in a year or two
without context, it will be very useful to have inline.
(Just a couple of sentences adapted from the second paragraph of the
commit message would do.)
> +{
> + struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
> + tree *split_loc = tp;
> +
> + /* Find the next split location within expression tree. */
> + tree *expr = NULL;
> + bool is_first = true;
> + while ((expr = find_next_split_location (split_loc)) != NULL)
> + {
> + gimple_seq before = NULL;
> +
> + if (*expr == *split_loc && is_first == true)
> + break;
> + is_first = false;
> +
> + push_gimplify_context ();
> + split_loc = &TREE_OPERAND (*expr, 0);
> +
> + tree rhs = *expr;
> + bool pointer_base = false;
> + if (!POINTER_TYPE_P (TREE_TYPE (rhs)))
> + {
> + rhs = fold_build1 (ADDR_EXPR,
> + build_pointer_type (TREE_TYPE (*expr)),
> + *expr);
> + pointer_base = true;
> + }
> + tree lhs = create_tmp_var_raw (TREE_TYPE (rhs), "core_split");
> + gimple_add_tmp_var (lhs);
> + gimplify_assign (lhs, rhs, &before);
> +
> + if (pointer_base == true)
> + lhs = fold_build2 (MEM_REF, TREE_TYPE (lhs), lhs,
> + build_int_cst (ptr_type_node, 0));
> + *expr = lhs;
> +
> + gsi_insert_seq_before (&(wi->gsi), before, GSI_NEW_STMT);
> + pop_gimplify_context (NULL);
> + }
> +
> + /* Never traverse subtrees, as find_next_split_location already does it. */
> + *walk_subtrees = false;
> +
> + /* Keep traverve all the other tree expressions in gimple. */
typo: traverse
> return NULL_TREE;
> }
>
> @@ -1851,10 +1889,11 @@ execute_lower_bpf_core (void)
> gimple_seq body = gimple_body (current_function_decl);
>
> struct walk_stmt_info wi;
> - core_access_clean ();
> -
> memset (&wi, 0, sizeof (wi));
> - wi.info = NULL;
> +
> + /* Early split to guarantee base of expression is a preserve_access_index
> + structure. */
> + walk_gimple_seq_mod (&body, NULL, gimple_core_early_split_expr, &wi);
>
> /* Split preserve_access_index expressions when needed. */
> walk_gimple_seq_mod (&body, NULL, make_gimple_core_safe_access_index, &wi);
> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> index d8725e4c5e20..5e8a1ac352de 100644
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -2498,7 +2498,7 @@ last_stmt_in_scope (gimple *stmt)
> if (!stmt)
> return NULL;
>
> - auto last_stmt_in_seq = [] (gimple_seq s)
> + auto last_stmt_in_seq = [] (gimple_seq s)
Some spurious change here in gimplify.cc that does not belong.
> {
> gimple_seq_node n;
> for (n = gimple_seq_last (s);
> diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-3.c
> b/gcc/testsuite/gcc.target/bpf/core-attr-3.c
> index 12354fc6f86d..58c27fd43bb4 100644
> --- a/gcc/testsuite/gcc.target/bpf/core-attr-3.c
> +++ b/gcc/testsuite/gcc.target/bpf/core-attr-3.c
> @@ -11,14 +11,14 @@
> struct O {
> int e;
> int f;
> -};
> +} __attribute__((preserve_access_index));
>
> struct S {
> int a;
> struct {
> int b;
> int c;
> - } inner;
> + } __attribute__((preserve_access_index)) inner;
> struct O other;
> } __attribute__((preserve_access_index));
>
> diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-4.c
> b/gcc/testsuite/gcc.target/bpf/core-attr-4.c
> index 6f025f42f3ee..c001b5b76ef9 100644
> --- a/gcc/testsuite/gcc.target/bpf/core-attr-4.c
> +++ b/gcc/testsuite/gcc.target/bpf/core-attr-4.c
> @@ -13,8 +13,8 @@ struct T {
> int d;
> int e[4];
> int f;
> - } v;
> - } u;
> + } __attribute__((preserve_access_index)) v;
> + } __attribute__((preserve_access_index)) u;
> } __attribute__((preserve_access_index));
>
>
> diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-5.c
> b/gcc/testsuite/gcc.target/bpf/core-attr-5.c
> index 81e25fa85de7..4d443d88b0ae 100644
> --- a/gcc/testsuite/gcc.target/bpf/core-attr-5.c
> +++ b/gcc/testsuite/gcc.target/bpf/core-attr-5.c
> @@ -11,7 +11,7 @@ struct U {
> int e[4];
> int f;
> int *g;
> - } v;
> + } __attribute__((preserve_access_index)) v;
> } __attribute__((preserve_access_index));
>
> struct T {
> diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-6.c
> b/gcc/testsuite/gcc.target/bpf/core-attr-6.c
> index 25215b5ae376..d43825ea97c3 100644
> --- a/gcc/testsuite/gcc.target/bpf/core-attr-6.c
> +++ b/gcc/testsuite/gcc.target/bpf/core-attr-6.c
> @@ -11,8 +11,8 @@ struct U {
> int e[4];
> int f;
> int *g;
> - } v;
> -} u;
> + } __attribute__((preserve_access_index)) v;
> +} __attribute__((preserve_access_index)) u;
>
> struct T {
> int a;
> diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-7.c
> b/gcc/testsuite/gcc.target/bpf/core-attr-7.c
> new file mode 100644
> index 000000000000..5ff8d9fcff95
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/bpf/core-attr-7.c
> @@ -0,0 +1,150 @@
> +/* Test for BPF CO-RE __attribute__((preserve_access_index)) with accesses on
> + LHS and both LHS and RHS of assignment. */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -dA -gbtf -mco-re -masm=normal" } */
> +
> +struct other
> +{
> + char c;
> + int i;
> +};
> +
> +struct inner_attr1
> +{
> + int i1;
> + int i2;
> +} __attribute__((preserve_access_index));
> +
> +struct inner_noattr
> +{
> + int i1;
> + int i2;
> +};
> +
> +union A_noattr
> +{
> + struct inner_attr1 inner_attr;
> + struct inner_noattr inner_no_attr;
> + struct inner_noattr *inner_p;
> +};
> +union A_attr
> +{
> + struct inner_attr1 inner_attr;
> + struct inner_noattr inner_no_attr;
> + struct inner_noattr *inner_p;
> +} __attribute__((preserve_access_index));
> +
> +
> +struct outer_noattr
> +{
> + struct other *other;
> + struct inner_attr
> + {
> + int i1;
> + int i2;
> + } __attribute__((preserve_access_index)) inner;
> + struct inner_noattr inner_no_attr;
> + struct inner_attr1 *inner_p;
> + union A_attr a_attr;
> + union A_noattr a_noattr;
> +};
> +
> +struct outer_attr
> +{
> + struct other *other;
> + struct inner_attr
> + {
> + int i1;
> + int i2;
> + } __attribute__((preserve_access_index)) inner;
> +
> + struct inner_noattr inner_no_attr;
> + struct inner_attr1 *inner_p;
> + union A_attr a_attr;
> + union A_noattr a_noattr;
> +} __attribute__((preserve_access_index));
> +
> +extern void use_value(int);
> +
> +void
> +func (struct outer_noattr *o, struct outer_attr *o_attr)
> +{
> + /* 0:1 */
> + o->inner.i2 = 7;
> + /* 0:1 */
> + o->inner_p->i2 = 8;
> + /* nothing */
> + o->inner_no_attr.i2 = 9;
> +
> + /* 0:2 */
> + o_attr->inner_no_attr.i2 = 10;
> +
> + /* nothing */
> + o->a_noattr.inner_no_attr.i1 = 1;
> + /* 0:1 */
> + o->a_attr.inner_no_attr.i1 = 2;
> + /* 0:0 */
> + o->a_noattr.inner_attr.i1 = 3;
> + /* 0:4:0:0 */
> + o_attr->a_attr.inner_attr.i1 = 4;
> + /* 0:5 0:0 */
> + o_attr->a_noattr.inner_attr.i1 = 5;
> +
> + /* This would still force everything as being attributed, although none of
> + the structs has the attribute. */
> + /* 0:5:2 0:0 */
> + __builtin_preserve_access_index (({ o->a_noattr.inner_p->i1 = 20; }));
> +
> + /* 0:2 */
> + struct inner_noattr d = o_attr->inner_no_attr;
> + use_value(d.i2);
> +}
> +
> +
> +extern void use(void *);
> +
> +struct udphdr {
> + int source;
> + int dest;
> + int len;
> + int check;
> +} __attribute__((preserve_access_index));
> +
> +union l4hdr {
> + struct udphdr udp;
> + int c;
> +};
> +
> +struct v4hdr {
> + int a;
> + union l4hdr l4hdr;
> + int b;
> +} __attribute__((packed));
> +
> +void gimple_loop_error(void)
> +{
> + struct v4hdr h_outer;
> + h_outer.l4hdr.udp.source = 1024;
> + use(&h_outer.l4hdr.udp.source);
> +}
> +
> +/* { dg-final { scan-assembler-times "ascii \"0:1.0\"\[\t
> \]+\[^\n\]*btf_aux_string" 1 } } */
> +/* { dg-final { scan-assembler-times "ascii \"0:0.0\"\[\t
> \]+\[^\n\]*btf_aux_string" 1 } } */
> +/* { dg-final { scan-assembler-times "ascii \"0:2.0\"\[\t
> \]+\[^\n\]*btf_aux_string" 1 } } */
> +/* { dg-final { scan-assembler-times "ascii \"0:5.0\"\[\t
> \]+\[^\n\]*btf_aux_string" 1 } } */
> +/* { dg-final { scan-assembler-times "ascii \"0:4:0:0.0\"\[\t
> \]+\[^\n\]*btf_aux_string" 1 } } */
> +/* { dg-final { scan-assembler-times "ascii \"0:5:2.0\"\[\t
> \]+\[^\n\]*btf_aux_string" 1 } } */
> +
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:0\"\\)" 4 } } */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:1\"\\)" 3 } } */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:2\"\\)" 2 } } */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:5\"\\)" 1 } } */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:4:0:0\"\\)" 1 }
> } */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:5:2\"\\)" 1 } }
> */
> +/* { dg-final { scan-assembler-times "bpfcr_type \\(struct inner_attr\\)" 1
> } } */
> +/* { dg-final { scan-assembler-times "bpfcr_type \\(struct inner_attr1\\)" 3
> } } */
> +/* { dg-final { scan-assembler-times "bpfcr_type \\(struct outer_attr\\)" 4
> } } */
> +/* { dg-final { scan-assembler-times "bpfcr_type \\(union A_attr\\)" 1 } } */
> +
> +
> diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-calls.c
> b/gcc/testsuite/gcc.target/bpf/core-attr-calls.c
> index 87290c5c2116..27b08af1bb79 100644
> --- a/gcc/testsuite/gcc.target/bpf/core-attr-calls.c
> +++ b/gcc/testsuite/gcc.target/bpf/core-attr-calls.c
> @@ -37,13 +37,13 @@ func (struct T *t, int i)
> /* 0:3 */
> get_other_u(t->ptr_u)->c = 43;
>
> - /* 0:2:1 */
> + /* 0:2 */
> get_other_v(&t->u.v)->d = 44;
> }
>
> /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:3\"\\)" 2 } } */
> /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:0\"\\)" 1 } } */
> -/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:2:1\"\\)" 1 } }
> */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:2\"\\)" 1 } } */
> /* { dg-final { scan-assembler-times "bpfcr_type \\(struct T\\)" 3 } } */
> /* { dg-final { scan-assembler-times "bpfcr_type \\(struct U\\)" 1 } } */
>
