Re: [PATCH 1/3] bpf: Fix CO-RE field expression builtins

[Cupertino Miranda <cupertino.miranda@oracle.com>]

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