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

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

[Cupertino Miranda]

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)));
+
+	    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 } } */
-- 
2.30.2

[PATCH 2/3] bpf: Fix access string default for CO-RE type based relocations

[Cupertino Miranda]

Although part of all CO-RE relocation data, type based relocations do
not require an access string.
Initial implementation defined it as an empty string.
On the other hand, libbpf when parsing the CO-RE relocations verifies
that those strings would contain "0", otherwise reports an error.
This patch makes GCC compliant with libbpf expectations.

gcc/Changelog:
	* config/bpf/btfext-out.cc (cpf_core_reloc_add): Correct for new code.
	Add assert to validate the string is set.
	* config/bpf/core-builtins.cc (cr_final): Make string struct
	field as const.
	(process_enum_value): Correct for field type change.
	(process_type): Set access string to "0".

gcc/testsuite/ChangeLog:
	* gcc.target/bpf/core-builtin-type-based.c: Correct.
	* gcc.target/bpf/core-builtin-type-id.c: Correct.
---
 gcc/config/bpf/btfext-out.cc                           |  5 +++--
 gcc/config/bpf/core-builtins.cc                        | 10 ++++++----
 gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c |  1 +
 gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c    |  1 +
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/gcc/config/bpf/btfext-out.cc b/gcc/config/bpf/btfext-out.cc
index 57c0dc323812..ff1fd0739f1e 100644
--- a/gcc/config/bpf/btfext-out.cc
+++ b/gcc/config/bpf/btfext-out.cc
@@ -299,8 +299,9 @@ bpf_core_reloc_add (const tree type, const char * section_name,
 
   /* Buffer the access string in the auxiliary strtab.  */
   bpfcr->bpfcr_astr_off = 0;
-  if (accessor != NULL)
-    bpfcr->bpfcr_astr_off = btf_ext_add_string (accessor);
+  gcc_assert (accessor != NULL);
+  bpfcr->bpfcr_astr_off = btf_ext_add_string (accessor);
+
   bpfcr->bpfcr_type = get_btf_id (ctf_lookup_tree_type (ctfc, type));
   bpfcr->bpfcr_insn_label = label;
   bpfcr->bpfcr_kind = kind;
diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
index 4256fea15e49..70b14e48e6e5 100644
--- a/gcc/config/bpf/core-builtins.cc
+++ b/gcc/config/bpf/core-builtins.cc
@@ -205,7 +205,7 @@ struct cr_local
 /* Core Relocation Final data */
 struct cr_final
 {
-  char *str;
+  const char *str;
   tree type;
   enum btf_core_reloc_kind kind;
 };
@@ -868,8 +868,10 @@ process_enum_value (struct cr_builtins *data)
 	{
 	  if (TREE_VALUE (l) == expr)
 	    {
-	      ret.str = (char *) ggc_alloc_atomic ((index / 10) + 1);
-	      sprintf (ret.str, "%d", index);
+	      char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1);
+	      sprintf (tmp, "%d", index);
+	      ret.str = (const char *) tmp;
+
 	      break;
 	    }
 	  index++;
@@ -987,7 +989,7 @@ process_type (struct cr_builtins *data)
 	      || data->kind == BPF_RELO_TYPE_MATCHES);
 
   struct cr_final ret;
-  ret.str = NULL;
+  ret.str = ggc_strdup ("0");
   ret.type = data->type;
   ret.kind = data->kind;
 
diff --git a/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c b/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c
index 74a8d5a14d9d..9d818133c084 100644
--- a/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c
+++ b/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c
@@ -56,3 +56,4 @@ int foo(void *data)
 /* { dg-final { scan-assembler-times "0x8\[\t \]+\[^\n\]*bpfcr_kind" 13 } } BPF_TYPE_EXISTS */
 /* { dg-final { scan-assembler-times "0x9\[\t \]+\[^\n\]*bpfcr_kind" 11 } } BPF_TYPE_SIZE */
 /* { dg-final { scan-assembler-times "0xc\[\t \]+\[^\n\]*bpfcr_kind" 13 } } BPF_TYPE_MATCHES */
+/* { dg-final { scan-assembler-times "bpfcr_astr_off \[(\"\]+0\[(\"\]+" 37 } } */
diff --git a/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c b/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c
index 4b23288eac08..9576b91bc940 100644
--- a/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c
+++ b/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c
@@ -38,3 +38,4 @@ int foo(void *data)
 /* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bpfcr_type" 0  { xfail *-*-* } } } */
 /* { dg-final { scan-assembler-times "0x6\[\t \]+\[^\n\]*bpfcr_kind" 13 } } BPF_TYPE_ID_LOCAL */
 /* { dg-final { scan-assembler-times "0x7\[\t \]+\[^\n\]*bpfcr_kind" 7 } } BPF_TYPE_ID_TARGET */
+/* { dg-final { scan-assembler-times "bpfcr_astr_off \[(\"\]+0\[(\"\]+" 20 } } */
-- 
2.30.2

[PATCH 3/3] bpf: Corrected index computation when present with unnamed struct fields

[Cupertino Miranda]

Any unnamed structure field if not a member of the BTF_KIND_STRUCT.
For that reason, CO-RE access strings indexes should take that in
consideration. This patch adds a condition to the incrementer that
computes the index for the field access.

gcc/ChangeLog:
	* config/bpf/core-builtins.cc (bpf_core_get_index): Check if
	field contains a DECL_NAME.

gcc/testsuite/ChangeLog:
	* gcc.target/bpf/core-builtin-fieldinfo-offset-1.c: Add
	testcase for unnamed fields.
---
 gcc/config/bpf/core-builtins.cc                        |  6 +++++-
 .../gcc.target/bpf/core-builtin-fieldinfo-offset-1.c   | 10 ++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
index 70b14e48e6e5..8333ad81d0e0 100644
--- a/gcc/config/bpf/core-builtins.cc
+++ b/gcc/config/bpf/core-builtins.cc
@@ -553,7 +553,11 @@ bpf_core_get_index (const tree node, bool *valid)
 	{
 	  if (l == node)
 	    return i;
-	  i++;
+	  /* Skip unnamed padding, not represented by BTF.  */
+	  if (DECL_NAME(l) != NULL_TREE
+	      || TREE_CODE (TREE_TYPE (l)) == UNION_TYPE
+	      || TREE_CODE (TREE_TYPE (l)) == RECORD_TYPE)
+	    i++;
 	}
     }
   else if (code == ARRAY_REF || code == ARRAY_RANGE_REF || code == MEM_REF)
diff --git a/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c b/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c
index 27654205287d..8b1d8b012a2a 100644
--- a/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c
+++ b/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c
@@ -14,6 +14,9 @@ struct T {
   struct S s[2];
   char c;
   char d;
+  int a: 1;
+  int:31;
+  int f;
 };
 
 enum {
@@ -38,7 +41,9 @@ unsigned int foo (struct T *t)
   unsigned e1 = __builtin_preserve_field_info (bar()->d, FIELD_BYTE_OFFSET);
   unsigned e2 = __builtin_preserve_field_info (bar()->s[1].a4, FIELD_BYTE_OFFSET);
 
-  return s0a1 + s0a4 + s0x + s1a1 + s1a4 + s1x + c + d + e1 + e2;
+  unsigned f1 = __builtin_preserve_field_info (t->f, FIELD_BYTE_OFFSET);
+
+  return s0a1 + s0a4 + s0x + s1a1 + s1a4 + s1x + c + d + e1 + e2 + f1;
 }
 
 /* { dg-final { scan-assembler-times "\[\t \]mov\[\t \]%r\[0-9\],4" 2 } } */
@@ -65,5 +70,6 @@ unsigned int foo (struct T *t)
 /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:1:1:4\"\\)" 1 } } */
 /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:2\"\\)" 1 } } */
 /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:3\"\\)" 2 } } */
+/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:5\"\\)" 1 } } */
 
-/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bpfcr_kind" 10 } } */
+/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bpfcr_kind" 11 } } */
-- 
2.30.2

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

[Jose E. Marchesi]

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

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

[Cupertino Miranda]

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

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

[Jose E. Marchesi]

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

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

[Jose E. Marchesi]

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

Re: [PATCH 2/3] bpf: Fix access string default for CO-RE type based relocations

[David Faust]

On 3/13/24 07:24, Cupertino Miranda wrote:
> Although part of all CO-RE relocation data, type based relocations do
> not require an access string.
> Initial implementation defined it as an empty string.
> On the other hand, libbpf when parsing the CO-RE relocations verifies
> that those strings would contain "0", otherwise reports an error.
> This patch makes GCC compliant with libbpf expectations.

OK, thanks.

> 
> gcc/Changelog:
> 	* config/bpf/btfext-out.cc (cpf_core_reloc_add): Correct for new code.
> 	Add assert to validate the string is set.
> 	* config/bpf/core-builtins.cc (cr_final): Make string struct
> 	field as const.
> 	(process_enum_value): Correct for field type change.
> 	(process_type): Set access string to "0".
> 
> gcc/testsuite/ChangeLog:
> 	* gcc.target/bpf/core-builtin-type-based.c: Correct.
> 	* gcc.target/bpf/core-builtin-type-id.c: Correct.
> ---
>  gcc/config/bpf/btfext-out.cc                           |  5 +++--
>  gcc/config/bpf/core-builtins.cc                        | 10 ++++++----
>  gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c |  1 +
>  gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c    |  1 +
>  4 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/config/bpf/btfext-out.cc b/gcc/config/bpf/btfext-out.cc
> index 57c0dc323812..ff1fd0739f1e 100644
> --- a/gcc/config/bpf/btfext-out.cc
> +++ b/gcc/config/bpf/btfext-out.cc
> @@ -299,8 +299,9 @@ bpf_core_reloc_add (const tree type, const char * section_name,
>  
>    /* Buffer the access string in the auxiliary strtab.  */
>    bpfcr->bpfcr_astr_off = 0;
> -  if (accessor != NULL)
> -    bpfcr->bpfcr_astr_off = btf_ext_add_string (accessor);
> +  gcc_assert (accessor != NULL);
> +  bpfcr->bpfcr_astr_off = btf_ext_add_string (accessor);
> +
>    bpfcr->bpfcr_type = get_btf_id (ctf_lookup_tree_type (ctfc, type));
>    bpfcr->bpfcr_insn_label = label;
>    bpfcr->bpfcr_kind = kind;
> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
> index 4256fea15e49..70b14e48e6e5 100644
> --- a/gcc/config/bpf/core-builtins.cc
> +++ b/gcc/config/bpf/core-builtins.cc
> @@ -205,7 +205,7 @@ struct cr_local
>  /* Core Relocation Final data */
>  struct cr_final
>  {
> -  char *str;
> +  const char *str;
>    tree type;
>    enum btf_core_reloc_kind kind;
>  };
> @@ -868,8 +868,10 @@ process_enum_value (struct cr_builtins *data)
>  	{
>  	  if (TREE_VALUE (l) == expr)
>  	    {
> -	      ret.str = (char *) ggc_alloc_atomic ((index / 10) + 1);
> -	      sprintf (ret.str, "%d", index);
> +	      char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1);
> +	      sprintf (tmp, "%d", index);
> +	      ret.str = (const char *) tmp;
> +
>  	      break;
>  	    }
>  	  index++;
> @@ -987,7 +989,7 @@ process_type (struct cr_builtins *data)
>  	      || data->kind == BPF_RELO_TYPE_MATCHES);
>  
>    struct cr_final ret;
> -  ret.str = NULL;
> +  ret.str = ggc_strdup ("0");
>    ret.type = data->type;
>    ret.kind = data->kind;
>  
> diff --git a/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c b/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c
> index 74a8d5a14d9d..9d818133c084 100644
> --- a/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c
> +++ b/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c
> @@ -56,3 +56,4 @@ int foo(void *data)
>  /* { dg-final { scan-assembler-times "0x8\[\t \]+\[^\n\]*bpfcr_kind" 13 } } BPF_TYPE_EXISTS */
>  /* { dg-final { scan-assembler-times "0x9\[\t \]+\[^\n\]*bpfcr_kind" 11 } } BPF_TYPE_SIZE */
>  /* { dg-final { scan-assembler-times "0xc\[\t \]+\[^\n\]*bpfcr_kind" 13 } } BPF_TYPE_MATCHES */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \[(\"\]+0\[(\"\]+" 37 } } */
> diff --git a/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c b/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c
> index 4b23288eac08..9576b91bc940 100644
> --- a/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c
> +++ b/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c
> @@ -38,3 +38,4 @@ int foo(void *data)
>  /* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bpfcr_type" 0  { xfail *-*-* } } } */
>  /* { dg-final { scan-assembler-times "0x6\[\t \]+\[^\n\]*bpfcr_kind" 13 } } BPF_TYPE_ID_LOCAL */
>  /* { dg-final { scan-assembler-times "0x7\[\t \]+\[^\n\]*bpfcr_kind" 7 } } BPF_TYPE_ID_TARGET */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \[(\"\]+0\[(\"\]+" 20 } } */

Re: [PATCH 3/3] bpf: Corrected index computation when present with unnamed struct fields

[David Faust]

On 3/13/24 07:24, Cupertino Miranda wrote:
> Any unnamed structure field if not a member of the BTF_KIND_STRUCT.
typo: if -> is

I'd suggest to clarify that "any unnamed structure field" is really
any unnamed non-struct-or-union field, since anonymous inner structs
and unions certainly are present in BTF (and you handle them here).

> For that reason, CO-RE access strings indexes should take that in
> consideration. This patch adds a condition to the incrementer that
> computes the index for the field access.

Otherwise, OK.
Thanks.

> 
> gcc/ChangeLog:
> 	* config/bpf/core-builtins.cc (bpf_core_get_index): Check if
> 	field contains a DECL_NAME.
> 
> gcc/testsuite/ChangeLog:
> 	* gcc.target/bpf/core-builtin-fieldinfo-offset-1.c: Add
> 	testcase for unnamed fields.
> ---
>  gcc/config/bpf/core-builtins.cc                        |  6 +++++-
>  .../gcc.target/bpf/core-builtin-fieldinfo-offset-1.c   | 10 ++++++++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
> index 70b14e48e6e5..8333ad81d0e0 100644
> --- a/gcc/config/bpf/core-builtins.cc
> +++ b/gcc/config/bpf/core-builtins.cc
> @@ -553,7 +553,11 @@ bpf_core_get_index (const tree node, bool *valid)
>  	{
>  	  if (l == node)
>  	    return i;
> -	  i++;
> +	  /* Skip unnamed padding, not represented by BTF.  */
> +	  if (DECL_NAME(l) != NULL_TREE
> +	      || TREE_CODE (TREE_TYPE (l)) == UNION_TYPE
> +	      || TREE_CODE (TREE_TYPE (l)) == RECORD_TYPE)
> +	    i++;
>  	}
>      }
>    else if (code == ARRAY_REF || code == ARRAY_RANGE_REF || code == MEM_REF)
> diff --git a/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c b/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c
> index 27654205287d..8b1d8b012a2a 100644
> --- a/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c
> +++ b/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c
> @@ -14,6 +14,9 @@ struct T {
>    struct S s[2];
>    char c;
>    char d;
> +  int a: 1;
> +  int:31;
> +  int f;
>  };
>  
>  enum {
> @@ -38,7 +41,9 @@ unsigned int foo (struct T *t)
>    unsigned e1 = __builtin_preserve_field_info (bar()->d, FIELD_BYTE_OFFSET);
>    unsigned e2 = __builtin_preserve_field_info (bar()->s[1].a4, FIELD_BYTE_OFFSET);
>  
> -  return s0a1 + s0a4 + s0x + s1a1 + s1a4 + s1x + c + d + e1 + e2;
> +  unsigned f1 = __builtin_preserve_field_info (t->f, FIELD_BYTE_OFFSET);
> +
> +  return s0a1 + s0a4 + s0x + s1a1 + s1a4 + s1x + c + d + e1 + e2 + f1;
>  }
>  
>  /* { dg-final { scan-assembler-times "\[\t \]mov\[\t \]%r\[0-9\],4" 2 } } */
> @@ -65,5 +70,6 @@ unsigned int foo (struct T *t)
>  /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:1:1:4\"\\)" 1 } } */
>  /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:2\"\\)" 1 } } */
>  /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:3\"\\)" 2 } } */
> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:5\"\\)" 1 } } */
>  
> -/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bpfcr_kind" 10 } } */
> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bpfcr_kind" 11 } } */

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

[Cupertino Miranda]

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

Re: [PATCH 2/3] bpf: Fix access string default for CO-RE type based relocations

[Cupertino Miranda]

David Faust writes:

> On 3/13/24 07:24, Cupertino Miranda wrote:
>> Although part of all CO-RE relocation data, type based relocations do
>> not require an access string.
>> Initial implementation defined it as an empty string.
>> On the other hand, libbpf when parsing the CO-RE relocations verifies
>> that those strings would contain "0", otherwise reports an error.
>> This patch makes GCC compliant with libbpf expectations.
>
> OK, thanks.
>
Pushed! Thanks
>>
>> gcc/Changelog:
>> 	* config/bpf/btfext-out.cc (cpf_core_reloc_add): Correct for new code.
>> 	Add assert to validate the string is set.
>> 	* config/bpf/core-builtins.cc (cr_final): Make string struct
>> 	field as const.
>> 	(process_enum_value): Correct for field type change.
>> 	(process_type): Set access string to "0".
>>
>> gcc/testsuite/ChangeLog:
>> 	* gcc.target/bpf/core-builtin-type-based.c: Correct.
>> 	* gcc.target/bpf/core-builtin-type-id.c: Correct.
>> ---
>>  gcc/config/bpf/btfext-out.cc                           |  5 +++--
>>  gcc/config/bpf/core-builtins.cc                        | 10 ++++++----
>>  gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c |  1 +
>>  gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c    |  1 +
>>  4 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/config/bpf/btfext-out.cc b/gcc/config/bpf/btfext-out.cc
>> index 57c0dc323812..ff1fd0739f1e 100644
>> --- a/gcc/config/bpf/btfext-out.cc
>> +++ b/gcc/config/bpf/btfext-out.cc
>> @@ -299,8 +299,9 @@ bpf_core_reloc_add (const tree type, const char * section_name,
>>
>>    /* Buffer the access string in the auxiliary strtab.  */
>>    bpfcr->bpfcr_astr_off = 0;
>> -  if (accessor != NULL)
>> -    bpfcr->bpfcr_astr_off = btf_ext_add_string (accessor);
>> +  gcc_assert (accessor != NULL);
>> +  bpfcr->bpfcr_astr_off = btf_ext_add_string (accessor);
>> +
>>    bpfcr->bpfcr_type = get_btf_id (ctf_lookup_tree_type (ctfc, type));
>>    bpfcr->bpfcr_insn_label = label;
>>    bpfcr->bpfcr_kind = kind;
>> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
>> index 4256fea15e49..70b14e48e6e5 100644
>> --- a/gcc/config/bpf/core-builtins.cc
>> +++ b/gcc/config/bpf/core-builtins.cc
>> @@ -205,7 +205,7 @@ struct cr_local
>>  /* Core Relocation Final data */
>>  struct cr_final
>>  {
>> -  char *str;
>> +  const char *str;
>>    tree type;
>>    enum btf_core_reloc_kind kind;
>>  };
>> @@ -868,8 +868,10 @@ process_enum_value (struct cr_builtins *data)
>>  	{
>>  	  if (TREE_VALUE (l) == expr)
>>  	    {
>> -	      ret.str = (char *) ggc_alloc_atomic ((index / 10) + 1);
>> -	      sprintf (ret.str, "%d", index);
>> +	      char *tmp = (char *) ggc_alloc_atomic ((index / 10) + 1);
>> +	      sprintf (tmp, "%d", index);
>> +	      ret.str = (const char *) tmp;
>> +
>>  	      break;
>>  	    }
>>  	  index++;
>> @@ -987,7 +989,7 @@ process_type (struct cr_builtins *data)
>>  	      || data->kind == BPF_RELO_TYPE_MATCHES);
>>
>>    struct cr_final ret;
>> -  ret.str = NULL;
>> +  ret.str = ggc_strdup ("0");
>>    ret.type = data->type;
>>    ret.kind = data->kind;
>>
>> diff --git a/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c b/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c
>> index 74a8d5a14d9d..9d818133c084 100644
>> --- a/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c
>> +++ b/gcc/testsuite/gcc.target/bpf/core-builtin-type-based.c
>> @@ -56,3 +56,4 @@ int foo(void *data)
>>  /* { dg-final { scan-assembler-times "0x8\[\t \]+\[^\n\]*bpfcr_kind" 13 } } BPF_TYPE_EXISTS */
>>  /* { dg-final { scan-assembler-times "0x9\[\t \]+\[^\n\]*bpfcr_kind" 11 } } BPF_TYPE_SIZE */
>>  /* { dg-final { scan-assembler-times "0xc\[\t \]+\[^\n\]*bpfcr_kind" 13 } } BPF_TYPE_MATCHES */
>> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \[(\"\]+0\[(\"\]+" 37 } } */
>> diff --git a/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c b/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c
>> index 4b23288eac08..9576b91bc940 100644
>> --- a/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c
>> +++ b/gcc/testsuite/gcc.target/bpf/core-builtin-type-id.c
>> @@ -38,3 +38,4 @@ int foo(void *data)
>>  /* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bpfcr_type" 0  { xfail *-*-* } } } */
>>  /* { dg-final { scan-assembler-times "0x6\[\t \]+\[^\n\]*bpfcr_kind" 13 } } BPF_TYPE_ID_LOCAL */
>>  /* { dg-final { scan-assembler-times "0x7\[\t \]+\[^\n\]*bpfcr_kind" 7 } } BPF_TYPE_ID_TARGET */
>> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \[(\"\]+0\[(\"\]+" 20 } } */

Re: [PATCH 3/3] bpf: Corrected index computation when present with unnamed struct fields

[Cupertino Miranda]

David Faust writes:

> On 3/13/24 07:24, Cupertino Miranda wrote:
>> Any unnamed structure field if not a member of the BTF_KIND_STRUCT.
> typo: if -> is
>
> I'd suggest to clarify that "any unnamed structure field" is really
> any unnamed non-struct-or-union field, since anonymous inner structs
> and unions certainly are present in BTF (and you handle them here).
>
>> For that reason, CO-RE access strings indexes should take that in
>> consideration. This patch adds a condition to the incrementer that
>> computes the index for the field access.
>
> Otherwise, OK.
> Thanks.

Corrected and Pushed. Thanks
>
>>
>> gcc/ChangeLog:
>> 	* config/bpf/core-builtins.cc (bpf_core_get_index): Check if
>> 	field contains a DECL_NAME.
>>
>> gcc/testsuite/ChangeLog:
>> 	* gcc.target/bpf/core-builtin-fieldinfo-offset-1.c: Add
>> 	testcase for unnamed fields.
>> ---
>>  gcc/config/bpf/core-builtins.cc                        |  6 +++++-
>>  .../gcc.target/bpf/core-builtin-fieldinfo-offset-1.c   | 10 ++++++++--
>>  2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/gcc/config/bpf/core-builtins.cc b/gcc/config/bpf/core-builtins.cc
>> index 70b14e48e6e5..8333ad81d0e0 100644
>> --- a/gcc/config/bpf/core-builtins.cc
>> +++ b/gcc/config/bpf/core-builtins.cc
>> @@ -553,7 +553,11 @@ bpf_core_get_index (const tree node, bool *valid)
>>  	{
>>  	  if (l == node)
>>  	    return i;
>> -	  i++;
>> +	  /* Skip unnamed padding, not represented by BTF.  */
>> +	  if (DECL_NAME(l) != NULL_TREE
>> +	      || TREE_CODE (TREE_TYPE (l)) == UNION_TYPE
>> +	      || TREE_CODE (TREE_TYPE (l)) == RECORD_TYPE)
>> +	    i++;
>>  	}
>>      }
>>    else if (code == ARRAY_REF || code == ARRAY_RANGE_REF || code == MEM_REF)
>> diff --git a/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c b/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c
>> index 27654205287d..8b1d8b012a2a 100644
>> --- a/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c
>> +++ b/gcc/testsuite/gcc.target/bpf/core-builtin-fieldinfo-offset-1.c
>> @@ -14,6 +14,9 @@ struct T {
>>    struct S s[2];
>>    char c;
>>    char d;
>> +  int a: 1;
>> +  int:31;
>> +  int f;
>>  };
>>
>>  enum {
>> @@ -38,7 +41,9 @@ unsigned int foo (struct T *t)
>>    unsigned e1 = __builtin_preserve_field_info (bar()->d, FIELD_BYTE_OFFSET);
>>    unsigned e2 = __builtin_preserve_field_info (bar()->s[1].a4, FIELD_BYTE_OFFSET);
>>
>> -  return s0a1 + s0a4 + s0x + s1a1 + s1a4 + s1x + c + d + e1 + e2;
>> +  unsigned f1 = __builtin_preserve_field_info (t->f, FIELD_BYTE_OFFSET);
>> +
>> +  return s0a1 + s0a4 + s0x + s1a1 + s1a4 + s1x + c + d + e1 + e2 + f1;
>>  }
>>
>>  /* { dg-final { scan-assembler-times "\[\t \]mov\[\t \]%r\[0-9\],4" 2 } } */
>> @@ -65,5 +70,6 @@ unsigned int foo (struct T *t)
>>  /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:1:1:4\"\\)" 1 } } */
>>  /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:2\"\\)" 1 } } */
>>  /* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:3\"\\)" 2 } } */
>> +/* { dg-final { scan-assembler-times "bpfcr_astr_off \\(\"0:5\"\\)" 1 } } */
>>
>> -/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bpfcr_kind" 10 } } */
>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bpfcr_kind" 11 } } */