> Jose E. Marchesi writes: > >>> This patch corrects bugs within the CO-RE builtin field expression >>> related builtins. >>> The following bugs were identified and corrected based on the expected >>> results of bpf-next selftests testsuite. >>> It addresses the following problems: >>> - Expressions with pointer dereferencing now point to the BTF structure >>> type, instead of the structure pointer type. >>> - Pointer addition to structure root is now identified and constructed >>> in CO-RE relocations as if it is an array access. For example, >>> "&(s+2)->b" generates "2:1" as an access string where "2" is >>> refering to the access for "s+2". >>> >>> gcc/ChangeLog: >>> * config/bpf/core-builtins.cc (core_field_info): Add >>> support for POINTER_PLUS_EXPR in the root of the field expression. >>> (bpf_core_get_index): Likewise. >>> (pack_field_expr): Make the BTF type to point to the structure >>> related node, instead of its pointer type. >>> (make_core_safe_access_index): Correct to new code. >>> >>> gcc/testsuite/ChangeLog: >>> * gcc.target/bpf/core-attr-5.c: Correct. >>> * gcc.target/bpf/core-attr-6.c: Likewise. >>> * gcc.target/bpf/core-attr-struct-as-array.c: Add test case for >>> pointer arithmetics as array access use case. >>> --- >>> gcc/config/bpf/core-builtins.cc | 54 +++++++++++++++---- >>> gcc/testsuite/gcc.target/bpf/core-attr-5.c | 4 +- >>> gcc/testsuite/gcc.target/bpf/core-attr-6.c | 4 +- >>> .../bpf/core-attr-struct-as-array.c | 35 ++++++++++++ >>> 4 files changed, 82 insertions(+), 15 deletions(-) >>> create mode 100644 gcc/testsuite/gcc.target/bpf/core-attr-struct-as-array.c >>> >>> diff --git a/gcc/config/bpf/core-builtins.cc >>> b/gcc/config/bpf/core-builtins.cc >>> index 8d8c54c1fb3d..4256fea15e49 100644 >>> --- a/gcc/config/bpf/core-builtins.cc >>> +++ b/gcc/config/bpf/core-builtins.cc >>> @@ -388,8 +388,8 @@ core_field_info (tree src, enum btf_core_reloc_kind >>> kind) >>> >>> src = root_for_core_field_info (src); >>> >>> - get_inner_reference (src, &bitsize, &bitpos, &var_off, &mode, &unsignedp, >>> - &reversep, &volatilep); >>> + tree root = get_inner_reference (src, &bitsize, &bitpos, &var_off, &mode, >>> + &unsignedp, &reversep, &volatilep); >>> >>> /* Note: Use DECL_BIT_FIELD_TYPE rather than DECL_BIT_FIELD here, >>> because it >>> remembers whether the field in question was originally declared as a >>> @@ -414,6 +414,23 @@ core_field_info (tree src, enum btf_core_reloc_kind >>> kind) >>> { >>> case BPF_RELO_FIELD_BYTE_OFFSET: >>> { >>> + result = 0; >>> + if (var_off == NULL_TREE >>> + && TREE_CODE (root) == INDIRECT_REF >>> + && TREE_CODE (TREE_OPERAND (root, 0)) == POINTER_PLUS_EXPR) >>> + { >>> + tree node = TREE_OPERAND (root, 0); >>> + tree offset = TREE_OPERAND (node, 1); >>> + tree type = TREE_TYPE (TREE_OPERAND (node, 0)); >>> + type = TREE_TYPE (type); >>> + >>> + gcc_assert (TREE_CODE (offset) == INTEGER_CST && tree_fits_shwi_p >>> (offset) >>> + && COMPLETE_TYPE_P (type) && tree_fits_shwi_p (TYPE_SIZE >>> (type))); >> >> What if an expression with a non-constant offset (something like s+foo) >> is passed to the builtin? Wouldn't it be better to error there instead >> of ICEing? >> > In that case, var_off == NULL_TREE, and it did not reach the assert. > In any case, please notice that this code was copied from some different > code in the same file which in that case would actually produce the > error earlier. The assert is there as a safe guard just in case the > other function stops detecting this case. > > In core-builtins.cc:572 > > else if (code == POINTER_PLUS_EXPR) > { > tree offset = TREE_OPERAND (node, 1); > tree type = TREE_TYPE (TREE_OPERAND (node, 0)); > type = TREE_TYPE (type); > > if (TREE_CODE (offset) == INTEGER_CST && tree_fits_shwi_p (offset) > && COMPLETE_TYPE_P (type) && tree_fits_shwi_p (TYPE_SIZE (type))) > { > HOST_WIDE_INT offset_i = tree_to_shwi (offset); > HOST_WIDE_INT type_size_i = tree_to_shwi (TYPE_SIZE_UNIT (type)); > if ((offset_i % type_size_i) == 0) > return offset_i / type_size_i; > } > } > > if (valid != NULL) > *valid = false; > return -1; > } > > Because the code, although similar, is actually having different > purposes, I decided not to abstract this in an independent function. My > perception was that it would be more confusing. > > Without wanting to paste too much code, please notice that the function > with the assert is only called if the above function, does not return > with error (i.e. valid != false).
Ok understood. Please submit upstream. Thanks! > >> >>> + >>> + HOST_WIDE_INT offset_i = tree_to_shwi (offset); >>> + result += offset_i; >>> + } >>> + >>> type = unsigned_type_node; >>> if (var_off != NULL_TREE) >>> { >>> @@ -422,9 +439,9 @@ core_field_info (tree src, enum btf_core_reloc_kind >>> kind) >>> } >>> >>> if (bitfieldp) >>> - result = start_bitpos / 8; >>> + result += start_bitpos / 8; >>> else >>> - result = bitpos / 8; >>> + result += bitpos / 8; >>> } >>> break; >>> >>> @@ -552,6 +569,7 @@ bpf_core_get_index (const tree node, bool *valid) >>> { >>> tree offset = TREE_OPERAND (node, 1); >>> tree type = TREE_TYPE (TREE_OPERAND (node, 0)); >>> + type = TREE_TYPE (type); >>> >>> if (TREE_CODE (offset) == INTEGER_CST && tree_fits_shwi_p (offset) >>> && COMPLETE_TYPE_P (type) && tree_fits_shwi_p (TYPE_SIZE (type))) >>> @@ -627,14 +645,18 @@ compute_field_expr (tree node, unsigned int >>> *accessors, >>> >>> switch (TREE_CODE (node)) >>> { >>> - case ADDR_EXPR: >>> - return 0; >>> case INDIRECT_REF: >>> - accessors[0] = 0; >>> - return 1; >>> - case POINTER_PLUS_EXPR: >>> - accessors[0] = bpf_core_get_index (node, valid); >>> - return 1; >>> + if (TREE_CODE (node = TREE_OPERAND (node, 0)) == POINTER_PLUS_EXPR) >>> + { >>> + accessors[0] = bpf_core_get_index (node, valid); >>> + *access_node = TREE_OPERAND (node, 0); >>> + return 1; >>> + } >>> + else >>> + { >>> + accessors[0] = 0; >>> + return 1; >>> + } >>> case COMPONENT_REF: >>> n = compute_field_expr (TREE_OPERAND (node, 0), accessors, >>> valid, >>> @@ -660,6 +682,7 @@ compute_field_expr (tree node, unsigned int *accessors, >>> access_node, false); >>> return n; >>> >>> + case ADDR_EXPR: >>> case CALL_EXPR: >>> case SSA_NAME: >>> case VAR_DECL: >>> @@ -688,6 +711,9 @@ pack_field_expr (tree *args, >>> tree access_node = NULL_TREE; >>> tree type = NULL_TREE; >>> >>> + if (TREE_CODE (root) == ADDR_EXPR) >>> + root = TREE_OPERAND (root, 0); >>> + >>> ret.reloc_decision = REPLACE_CREATE_RELOCATION; >>> >>> unsigned int accessors[100]; >>> @@ -695,6 +721,8 @@ pack_field_expr (tree *args, >>> compute_field_expr (root, accessors, &valid, &access_node, false); >>> >>> type = TREE_TYPE (access_node); >>> + if (POINTER_TYPE_P (type)) >>> + type = TREE_TYPE (type); >>> >>> if (valid == true) >>> { >>> @@ -1351,6 +1379,8 @@ make_core_safe_access_index (tree expr, bool >>> *changed, bool entry = true) >>> if (base == NULL_TREE || base == expr) >>> return expr; >>> >>> + base = expr; >>> + >>> tree ret = NULL_TREE; >>> int n; >>> bool valid = true; >>> @@ -1365,6 +1395,8 @@ make_core_safe_access_index (tree expr, bool >>> *changed, bool entry = true) >>> { >>> if (TREE_CODE (access_node) == INDIRECT_REF) >>> base = TREE_OPERAND (access_node, 0); >>> + else >>> + base = access_node; >>> >>> bool local_changed = false; >>> ret = make_core_safe_access_index (base, &local_changed, false); >>> diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-5.c >>> b/gcc/testsuite/gcc.target/bpf/core-attr-5.c >>> index e71901d0d4d1..90734dab3a29 100644 >>> --- a/gcc/testsuite/gcc.target/bpf/core-attr-5.c >>> +++ b/gcc/testsuite/gcc.target/bpf/core-attr-5.c >>> @@ -63,5 +63,5 @@ func (struct T *t, int i) >>> /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:1:2\"\\)" 1 } >>> } */ >>> /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:1:1:1\"\\)" 1 >>> } } */ >>> /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:0\"\\)" 1 } } >>> */ >>> -/* { dg-final { scan-assembler-times "bpfcr_type \\(struct T \\*\\)" 4 } } >>> */ >>> -/* { dg-final { scan-assembler-times "bpfcr_type \\(struct U \\*\\)" 4 { >>> xfail *-*-* } } } */ >>> +/* { dg-final { scan-assembler-times "bpfcr_type \\(struct T\\)" 4 } } */ >>> +/* { dg-final { scan-assembler-times "bpfcr_type \\(struct U\\)" 4 { xfail >>> *-*-* } } } */ >>> diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-6.c >>> b/gcc/testsuite/gcc.target/bpf/core-attr-6.c >>> index 34a4c367e528..d0c5371b86e0 100644 >>> --- a/gcc/testsuite/gcc.target/bpf/core-attr-6.c >>> +++ b/gcc/testsuite/gcc.target/bpf/core-attr-6.c >>> @@ -45,6 +45,6 @@ func (struct T *t, int i) >>> /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:3\"\\)" 2 } } >>> */ >>> /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:1:2\"\\)" 1 } >>> } */ >>> /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:0\"\\)" 1 } } >>> */ >>> -/* { dg-final { scan-assembler-times "bpfcr_type \\(struct T \\*\\)" 3 } } >>> */ >>> -/* { dg-final { scan-assembler-times "bpfcr_type \\(struct U \\*\\)" 2 } } >>> */ >>> +/* { dg-final { scan-assembler-times "bpfcr_type \\(struct T\\)" 3 } } */ >>> +/* { dg-final { scan-assembler-times "bpfcr_type \\(struct U\\)" 2 } } */ >>> >>> diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-struct-as-array.c >>> b/gcc/testsuite/gcc.target/bpf/core-attr-struct-as-array.c >>> new file mode 100644 >>> index 000000000000..3f6eb9cb97f8 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.target/bpf/core-attr-struct-as-array.c >>> @@ -0,0 +1,35 @@ >>> +/* Basic test for struct __attribute__((preserve_access_index)) >>> + for BPF CO-RE support. */ >>> + >>> +/* { dg-do compile } */ >>> +/* { dg-options "-O0 -dA -gbtf -mco-re" } */ >>> + >>> +struct S { >>> + int a; >>> + int b; >>> + int c; >>> +} __attribute__((preserve_access_index)); >>> + >>> +void >>> +func (struct S * s) >>> +{ >>> + /* This test is marked as XFAIL since for the time being the CO-RE >>> + implementation is not able to disambiguate between a point >>> manipulation >>> + and a CO-RE access when using preserve_access_index attribute. The >>> + current implemetantion is incorrect if we consider that STRUCT S might >>> + have different size within the kernel. >>> + This example demonstrates how the implementation of >>> preserve_access_index >>> + as an attribute of the type is flagile. */ >>> + >>> + /* 2:2 */ >>> + int *x = &((s+2)->c); >>> + *x = 4; >>> + >>> + /* 2:1 */ >>> + int *y = __builtin_preserve_access_index (&((s+2)->b)); >>> + *y = 2; >>> +} >>> + >>> +/* { dg-final { scan-assembler-times "ascii \"2:2.0\"\[\t >>> \]+\[^\n\]*btf_aux_string" 1 { xfail *-*-* } } } */ >>> +/* { dg-final { scan-assembler-times "ascii \"2:1.0\"\[\t >>> \]+\[^\n\]*btf_aux_string" 1 } } */ >>> +/* { dg-final { scan-assembler-times "bpfcr_type" 2 } } */