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