On 4/2/26 08:31, Cupertino Miranda wrote:
> Hi everyone,
>
> This patch fixes bit-field accesses in CO-RE code generation.
>
> No regressions. The selftests improved with following diff:
>
> 3993c3993
> < #463 tc_change_tail:FAIL
> ---
>> #463 tc_change_tail:OK
> 4013c4013
> < #473/4 tc_tunnel/sit_none:FAIL
> ---
>> #473/4 tc_tunnel/sit_none:OK
> 4028c4028
> < #473 tc_tunnel:FAIL
> ---
>> #473 tc_tunnel:OK
>
> Looking forward to your review.
Nice :)
At this point you are definitely the expert on the CO-RE handling
code, but the approach makes sense to me and I don't spot any
big problems.
A few comments inline below.
At some point I remember we discussed an issue when the bitfield
access is wrapped by __builtin_preserve_access_index on the LHS
of an assign, but it looks like that case isn't covered in the
test. Was that resolved? Would also be good to add in the test.
And I agree with Jose, if it's possible please add some execution
tests similar to the (previously) failing kernel selftests so we
can avoid regressions.
>
> Cheers,
> Cupertino
>
> ------
>
> This patch corrects GCC to do proper bit shifting for bit-field access
> expressions that get patched for CO-RE access.
> Since we convert field accesses in an early pass, we needed to
> explicitly generate shifting and masking to properly access the
> bit-fields.
>
> gcc/ChangeLog:
>
> PR target/123894
> * config/bpf/core-builtins.cc
> (core_expr_with_field_expr_plus_base): Add parameter to return
> the tree node for the type associated with the access mode.
> (gen_mask_for_size): New function
> (gen_rhs_bitfield_adjust): Add function to adapt bitfield accesses
> through shifting and masking.
> (gen_lrhs_bitfield_adjust): Add function to adapt bitfields in rhs
> when a core expression is used in a lhs assignment.
> (requires_bitfield_adjust): Add function to verify if an
> expression requires bitfield adaptation.
> (replace_core_access_index_comp_expr): Add logic for bitfield
> adaptation.
> (core_make_builtins): Refactor gimplification. Do bitfield
> adjusting.
>
> gcc/testsuite/
>
> * gcc.target/bpf/core-bitfields.c: Added new test.
> ---
> gcc/config/bpf/core-builtins.cc | 238 +++++++++++++++---
> gcc/testsuite/gcc.target/bpf/core-bitfields.c | 57 +++++
> 2 files changed, 261 insertions(+), 34 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/bpf/core-bitfields.c
>
> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
> index d0c6cfdaf1c0..7b994fd6f10b 100644
> --- a/gcc/config/bpf/core-builtins.cc
> +++ b/gcc/config/bpf/core-builtins.cc
> @@ -1328,11 +1328,23 @@ construct_builtin_core_reloc (location_t loc, tree
> fndecl, tree *args,
> - base + __builtin_preserve_field_expr (expr, SIZEOF) */
>
> static tree
> -core_expr_with_field_expr_plus_base (tree base, tree expr, bool leaf_node)
> +core_expr_with_field_expr_plus_base (tree base, tree expr, bool leaf_node,
> + tree *access_type)
> {
> tree type = TREE_TYPE (expr);
> tree args[2];
>
> + tree mem_access_type = lang_hooks.types.type_for_mode (TYPE_MODE (type),
> + TYPE_UNSIGNED (type));
> +
> + /* Only use mode type for integer types, where the value can be a bitfield
> + access. */
> + if (TREE_CODE (type) != INTEGER_TYPE)
> + mem_access_type = type;
> +
> + mem_access_type = build_pointer_type (mem_access_type);
> + *access_type = mem_access_type;
Maybe I missed it, but I don't see this new *access_type param
actually getting used by any of the callers. Maybe it's left
from an older iteration?
If it is used but only in a minority of cases then I suggest to
wrap the above with a null check and let callers who don't
care about the result just pass null.
> +
> if (base == expr)
> return expr;
> else if (TREE_CODE (expr) == ARRAY_REF
> @@ -1366,12 +1378,13 @@ core_expr_with_field_expr_plus_base (tree base, tree
> expr, bool leaf_node)
> fold_build1 (NOP_EXPR, ptr_type_node, base),
> offset);
>
> - tmp = fold_build1 (NOP_EXPR, build_pointer_type (type), tmp);
> + tmp = fold_build1 (NOP_EXPR, mem_access_type, tmp);
> return tmp;
> }
> else
> {
> tree fndecl = bpf_builtins[BPF_BUILTIN_PRESERVE_FIELD_INFO];
> +
> args[0] = expr;
> args[1] = build_int_cst (integer_type_node, BPF_FIELD_BYTE_OFFSET);
> tree builtin_call = construct_builtin_core_reloc (UNKNOWN_LOCATION,
> @@ -1385,7 +1398,7 @@ core_expr_with_field_expr_plus_base (tree base, tree
> expr, bool leaf_node)
> tree tmp = fold_build2 (POINTER_PLUS_EXPR, ptr_type_node,
> fold_build1 (NOP_EXPR, ptr_type_node, base),
> fold_build1 (NOP_EXPR, size_type_node, builtin_call));
> - tmp = fold_build1 (NOP_EXPR, build_pointer_type (type), tmp);
> + tmp = fold_build1 (NOP_EXPR, mem_access_type, tmp);
> return tmp;
> }
> }
> @@ -1460,8 +1473,10 @@ make_core_safe_access_index (tree expr, bool *changed,
> bool entry = true)
> /* The remaining is to traverse the field part of the field expression. */
> if (mode != VOIDmode && var_off == NULL_TREE)
> {
> + tree access_type = NULL_TREE;
> *changed = true;
> - return core_expr_with_field_expr_plus_base (base, expr, true);
> + return core_expr_with_field_expr_plus_base (base, expr, true,
> + &access_type);
> }
> else
> {
> @@ -1496,8 +1511,10 @@ make_core_safe_access_index (tree expr, bool *changed,
> bool entry = true)
> && mode != VOIDmode && var_off != NULL_TREE)
> || entry == true)
> {
> + tree access_type = NULL_TREE;
> *changed = true;
> - ret = core_expr_with_field_expr_plus_base (ret, expr, false);
> + ret = core_expr_with_field_expr_plus_base (ret, expr, false,
> + &access_type);
> }
> return ret;
> }
> @@ -1507,12 +1524,129 @@ make_core_safe_access_index (tree expr, bool
> *changed, bool entry = true)
> __builtin_preserve_access_index argument expression from within
> bpf_resolve_overloaded_core_builtin. */
>
> +static tree
> +gen_mask_for_size (tree type, poly_int64 bitsize)
> +{
> + tree mask = build_int_cst (type, 1);
> + mask = fold_build2 (LSHIFT_EXPR, type, mask,
> + build_int_cst (type, bitsize));
> + mask = fold_build2 (MINUS_EXPR, type, mask,
> + build_int_cst (type, 1));
> + return mask;
> +}
Please add a function-level comment for each of these new functions.
> +
> +static tree
> +gen_rhs_bitfield_adjust (tree bit_type, tree mem_access_type,
> + tree mem_ref, tree offset, tree sizemask)
> +{
> + tree value = mem_ref;
> +
> + /* Shift read value to position it in least significant bits. */
> + if (TREE_INT_CST_LOW (offset) != 0)
> + value = fold_build2 (RSHIFT_EXPR, mem_access_type, value, offset);
> +
> + value = fold_build2 (BIT_AND_EXPR, mem_access_type, value, sizemask);;
> + /* The casting takes care of the sign extension. */
> + value = fold_build1 (NOP_EXPR, bit_type, value);
> + return value;
> +}
> +
> +static tree
> +gen_lrhs_bitfied_adjust (tree mem_access_type, tree mem_ref, tree lhs,
> + tree offset, tree bitmask)
> +{
> + /* Mask to clear bits in destination bitfield. */
> + tree clear_mask = fold_build2 (LSHIFT_EXPR, mem_access_type, bitmask,
> offset);
> + clear_mask = fold_build1 (BIT_NOT_EXPR, mem_access_type, clear_mask);
> +
> + /* Collect the value from the pointer reference. */
> + tree read_value = fold_build2 (BIT_AND_EXPR, mem_access_type, mem_ref,
> + clear_mask);
> +
> +
> + tree new_value = lhs;
> + if (TREE_CODE (lhs) == NOP_EXPR
> + && TREE_TYPE (TREE_OPERAND (lhs, 0)) == mem_access_type)
> + new_value = TREE_OPERAND (lhs, 0);
> + new_value = fold_build1 (NOP_EXPR, mem_access_type, new_value);
> + new_value = fold_build2 (BIT_AND_EXPR, mem_access_type, new_value,
> bitmask);
> + new_value = fold_build2 (LSHIFT_EXPR, mem_access_type, new_value, offset);
> +
> +
> + new_value = fold_build2 (BIT_IOR_EXPR, mem_access_type, read_value,
> + new_value);
> + return new_value;
> +}
> +
> +static bool
> +requires_bitfield_adjust (tree node, tree *p_mem_access_type,
> + tree *offset, tree *bitmask)
> +{
> + poly_int64 bitsize, bitpos;
> + tree var_off;
> + machine_mode mode;
> + int sign, reverse, vol;
> +
> + get_inner_reference (node, &bitsize, &bitpos, &var_off, &mode,
> + &sign, &reverse, &vol);
> +
> + tree type = TREE_TYPE (node);
> + tree mem_access_type = lang_hooks.types.type_for_mode (TYPE_MODE (type),
> + TYPE_UNSIGNED (type));
> + *p_mem_access_type = mem_access_type;
> +
> + /* Only use mode type for integer types, where the value can be a bitfield
> + access. */
> + if (TREE_CODE (type) != INTEGER_TYPE)
> + return false;
> +
> + if (TYPE_MAIN_VARIANT (type) != TYPE_MAIN_VARIANT (mem_access_type))
> + {
> + *offset = fold_build2 (TRUNC_MOD_EXPR, mem_access_type,
> + build_int_cst (mem_access_type, bitpos),
> + TYPE_SIZE (mem_access_type));
> +
> + *bitmask = gen_mask_for_size (mem_access_type, bitsize);
> +
> + return true;
> + }
> +
> + return false;
> +}
> +
> static tree
> replace_core_access_index_comp_expr (tree *node, int *walk_subtrees,
> - void *data ATTRIBUTE_UNUSED)
> + void *data)
> {
> bool valid = true;
> gcc_assert (*node != NULL_TREE);
> + bool is_lhs = (bool) data;
> + tree mem_access_type, offset, bitmask;
> +
> + if (TREE_CODE (*node) == MODIFY_EXPR
> + && requires_bitfield_adjust (TREE_OPERAND (*node, 0), &mem_access_type,
> + &offset, &bitmask))
> + {
> + TREE_VISITED (*node) = 1;
> + walk_tree (&TREE_OPERAND (*node, 0),
> replace_core_access_index_comp_expr,
> + (void *) true, NULL);
> + walk_tree (&TREE_OPERAND (*node, 1),
> replace_core_access_index_comp_expr,
> + NULL, NULL);
> +
> + TREE_TYPE (*node) = TREE_TYPE (TREE_OPERAND (*node, 0));
> + TREE_OPERAND (*node, 1) = gen_lrhs_bitfied_adjust (mem_access_type,
> + TREE_OPERAND (*node, 0),
> + TREE_OPERAND (*node, 1),
> + offset, bitmask);
> +
> + *node = fold_build2 (MODIFY_EXPR,
> + TREE_TYPE (TREE_OPERAND (*node, 0)),
> + TREE_OPERAND (*node, 0),
> + TREE_OPERAND (*node, 1));
> +
> + *walk_subtrees = false;
Here you assign walk_subtrees as a bool, but lower down in
the patch you change from assigning 'false' to assigning 0.
It works, but would be better to pick one and be consistent :)
(IMO: use a bool and true/false)
> + return NULL_TREE;
> + }
>
> tree *expr = node;
> bool should_indirect = false;
> @@ -1525,18 +1659,29 @@ replace_core_access_index_comp_expr (tree *node, int
> *walk_subtrees,
> if (valid == true && n > 0)
> {
> bool changed = false;
> - tree expr_test = make_core_safe_access_index (*expr, &changed);
> - *walk_subtrees = 0;
> + tree expr_test = make_core_safe_access_index (*expr, &changed, true);
> + tree access_type = TREE_TYPE (expr_test);
>
> if (changed == true)
> {
> if (should_indirect == true)
> expr_test = fold_build1 (INDIRECT_REF,
> - TREE_TYPE (TREE_TYPE (expr_test)),
> + TREE_TYPE (access_type),
> expr_test);
>
> + if (is_lhs == false
> + && requires_bitfield_adjust (*node, &mem_access_type, &offset,
> + &bitmask))
> + {
> + tree orig_type = TREE_TYPE (*node);
> + expr_test = gen_rhs_bitfield_adjust (orig_type, mem_access_type,
> + expr_test, offset, bitmask);
> + }
> +
> +
> *expr = expr_test;
> }
> + *walk_subtrees = 0;
> }
> return NULL_TREE;
> }
> @@ -1896,49 +2041,74 @@ core_make_builtins (tree *tp, int *walk_subtrees,
> void *data)
> expr = TREE_OPERAND (*tp, 0);
> }
>
> + poly_int64 bitsize, bitpos;
> + tree var_off;
> + machine_mode mode;
> + int sign, reverse, vol;
> +
> + tree base = get_inner_reference (expr, &bitsize, &bitpos, &var_off, &mode,
> + &sign, &reverse, &vol);
> +
> if (is_attr_preserve_access (expr)
> && (n = compute_field_expr (expr, NULL, &valid, NULL)) > 0
> && valid == true)
> {
> - poly_int64 bitsize, bitpos;
> - tree var_off;
> - machine_mode mode;
> - int sign, reverse, vol;
> -
> - tree base = get_inner_reference (expr, &bitsize, &bitpos, &var_off,
> &mode,
> - &sign, &reverse, &vol);
> -
> - tree new_expr = core_expr_with_field_expr_plus_base (base, expr, true);
> + tree access_type = NULL_TREE;
> + tree new_expr = core_expr_with_field_expr_plus_base (base, expr, true,
> + &access_type);
>
> /* Abort convertion if it is just a pointer or a reference to an
> attributed type. */
> if (new_expr != expr)
> {
> -
> gimple_seq before = NULL;
> + gimple_seq after = NULL;
> push_gimplify_context ();
> -
> gimplify_expr (&new_expr, &before, NULL, is_gimple_val, fb_rvalue);
>
> tree new_expr_type = TREE_TYPE (new_expr);
> -
> - /* Replace original expression by new_expr. If the type is the same
> - tree node, good! If it is a pointer type, we need to dereference
> - the type within the pointer to guarantee it is the same. */
> - if (TREE_TYPE (new_expr) == TREE_TYPE (*tp)
> - || (is_addr_expr
> - && TREE_TYPE (new_expr_type) == TREE_TYPE (expr)))
> - *tp = fold (new_expr);
> - else if (TREE_TYPE (new_expr) == build_pointer_type (TREE_TYPE (*tp)))
> - *tp = fold_build2 (MEM_REF, TREE_TYPE (*tp),
> - new_expr, build_int_cst (ptr_type_node, 0));
> + tree mem_access_type, offset, bitmask;
> +
> + if (requires_bitfield_adjust (*tp , &mem_access_type, &offset,
> + &bitmask))
> + {
> + tree value = build_simple_mem_ref (new_expr);
> + if (wi->is_lhs)
> + {
> + gcc_assert (is_gimple_assign (wi->stmt));
> + tree *rhs = gimple_assign_rhs1_ptr (wi->stmt);
> +
> + *rhs = gen_lrhs_bitfied_adjust (mem_access_type,
> + value, *rhs, offset,
> + bitmask);
> + gimplify_expr (rhs, &before, &after, is_gimple_val,
> + fb_rvalue);
> + }
> + else
> + {
> + value = gen_rhs_bitfield_adjust (TREE_TYPE (*tp),
> + mem_access_type,
> + value, offset, bitmask);
> + gimplify_expr (&value, &before, NULL, is_gimple_val,
> + fb_rvalue);
> + }
> + *tp = value;
> + }
> else
> - gcc_assert (0);
> + {
> + if (TYPE_MAIN_VARIANT (new_expr_type) != TYPE_MAIN_VARIANT (
> + TREE_TYPE (*tp))
> + && !is_addr_expr)
> + new_expr = build_simple_mem_ref (new_expr);
> +
> + new_expr = fold (new_expr);
> + *tp = new_expr;
> + }
>
> gsi_insert_seq_before (&(wi->gsi), before, GSI_SAME_STMT);
> + gsi_insert_seq_after (&(wi->gsi), after, GSI_SAME_STMT);
> pop_gimplify_context (NULL);
> -
> - *walk_subtrees = false;
> + *walk_subtrees = 0;
> }
> }
> return NULL_TREE;
> diff --git a/gcc/testsuite/gcc.target/bpf/core-bitfields.c
> b/gcc/testsuite/gcc.target/bpf/core-bitfields.c
> new file mode 100644
> index 000000000000..6cbaf1035fb4
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/bpf/core-bitfields.c
> @@ -0,0 +1,57 @@
> +/* 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 S {
> + unsigned char a: 2;
> + unsigned char b: 2;
> + unsigned char c: 2;
> + unsigned char d: 2;
> +} __attribute__((preserve_access_index));
> +
> +struct S_no {
> + unsigned char a: 2;
> + unsigned char b: 2;
> + unsigned char c: 2;
> + unsigned char d: 2;
> +};
> +
> +char foo (struct S *s, struct S_no *s_no, unsigned char v)
> +{
> + unsigned char z = s->c;
> + s->b = v;
> +
> + return z;
> +}
> +char bar (struct S_no *s_no)
> +{
> + unsigned char y = __builtin_preserve_access_index (s_no->d);
> + return y;
> +}
> +
> +/* { dg-final { scan-assembler-times "ascii \"0:1.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:3.0\"\[\t
> \]+\[^\n\]*btf_aux_string" 1 } } */
> +
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:1\"\\)" 1 } } */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:2\"\\)" 1 } } */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:3\"\\)" 1 } } */
> +/* { dg-final { scan-assembler-times "bpfcr_type \\(struct S\\)" 2 } } */
> +/* { dg-final { scan-assembler-times "bpfcr_type \\(struct S_no\\)" 1 } } */
> +
> +/* In foo */
> +
> +/* { dg-final { scan-assembler-times "rsh32\t%r0,4" 1 } } */
> +/* { dg-final { scan-assembler-times "lsh32\t%r3,2" 1 } } */
> +/* { dg-final { scan-assembler-times "and32\t%r3,12" 1 } } */
> +/* { dg-final { scan-assembler-times "and32\t%r4,-13" 1 } } */
> +/* { dg-final { scan-assembler-times "and32\t%r0,3" 1 } } */
> +/* { dg-final { scan-assembler-times "or32\t%r4,%r3" 1 } } */
> +
> +/* In bar */
> +
> +/* { dg-final { scan-assembler-times "rsh32\t%r0,6" 1 } } */
> +
> +
