| Message ID | 20240313142441.180242-1-cupertino.miranda@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] bpf: Fix CO-RE field expression builtins | expand |
> 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? > + > + 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 } } */
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). > >> + >> + 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 } } */
> 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 } } */
>> 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! Heh this is already upstream, sorry. The patch is OK. 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 } } */
>>>>> 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! > > Heh this is already upstream, sorry. > The patch is OK. > Thanks! Pushed ! 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 } } */
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))); + + 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 } } */