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