>>>>> 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 } } */
