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.
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;
+
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;
+}
+
+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;
+ 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 } } */
+
+
--
2.47.3
