diff mbox series

[v2] bpf: make sure CO-RE relocs are typed with struct BTF_KIND_STRUCT

Message ID 20241014180429.49963-1-cupertino.miranda@oracle.com
State New
Headers show
Series [v2] bpf: make sure CO-RE relocs are typed with struct BTF_KIND_STRUCT | expand

Commit Message

Cupertino Miranda Oct. 14, 2024, 6:04 p.m. UTC 
Hi everyone,

Here is the v2 for the patch in this thread:
  https://gcc.gnu.org/pipermail/gcc-patches/2024-October/665378.html
Please noticed that commit message was adapted to new content.

Regards,
Cupertino

Based on observation within bpf-next selftests and comparisson of GCC
and clang compiled code, the BPF loader expects all CO-RE relocations to
point to BTF non const and non volatile type nodes.
---
 gcc/btfout.cc                                 |  2 +-
 gcc/config/bpf/btfext-out.cc                  |  7 ++++
 gcc/ctfc.h                                    |  2 +
 .../gcc.target/bpf/core-attr-const.c          | 40 +++++++++++++++++++
 4 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/bpf/core-attr-const.c

Comments

David Faust Oct. 15, 2024, 6:51 p.m. UTC | #1 
On 10/14/24 11:04, Cupertino Miranda wrote:
> Hi everyone,
> 
> Here is the v2 for the patch in this thread:
>   https://gcc.gnu.org/pipermail/gcc-patches/2024-October/665378.html
> Please noticed that commit message was adapted to new content.
> 
> Regards,
> Cupertino

Hi Cupertino,

Thanks for the patch. The fix LGTM.

I have some comments about the new test below. But it is ok for now, and
I think addressing them properly is out of scope of this patch. This fix
will be good to have. So the patch is OK.

> 
> Based on observation within bpf-next selftests and comparisson of GCC
> and clang compiled code, the BPF loader expects all CO-RE relocations to
> point to BTF non const and non volatile type nodes.
> ---
>  gcc/btfout.cc                                 |  2 +-
>  gcc/config/bpf/btfext-out.cc                  |  7 ++++
>  gcc/ctfc.h                                    |  2 +
>  .../gcc.target/bpf/core-attr-const.c          | 40 +++++++++++++++++++
>  4 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/bpf/core-attr-const.c
> 
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index 8b91bde8798..24f62ec1a52 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -167,7 +167,7 @@ get_btf_kind (uint32_t ctf_kind)
>  
>  /* Convenience wrapper around get_btf_kind for the common case.  */
>  
> -static uint32_t
> +uint32_t
>  btf_dtd_kind (ctf_dtdef_ref dtd)
>  {
>    if (!dtd)
> diff --git a/gcc/config/bpf/btfext-out.cc b/gcc/config/bpf/btfext-out.cc
> index 095c35b894b..30266a2d384 100644
> --- a/gcc/config/bpf/btfext-out.cc
> +++ b/gcc/config/bpf/btfext-out.cc
> @@ -320,6 +320,13 @@ bpf_core_reloc_add (const tree type, const char * section_name,
>    ctf_container_ref ctfc = ctf_get_tu_ctfc ();
>    ctf_dtdef_ref dtd = ctf_lookup_tree_type (ctfc, type);
>  
> +  /* Make sure CO-RE type is never the const or volatile version.  */
> +  if ((btf_dtd_kind (dtd) == BTF_KIND_CONST
> +       || btf_dtd_kind (dtd) == BTF_KIND_VOLATILE)
> +      && kind >= BPF_RELO_FIELD_BYTE_OFFSET
> +      && kind <= BPF_RELO_FIELD_RSHIFT_U64)
> +    dtd = dtd->ref_type;
> +
>    /* Buffer the access string in the auxiliary strtab.  */
>    bpfcr->bpfcr_astr_off = 0;
>    gcc_assert (accessor != NULL);
> diff --git a/gcc/ctfc.h b/gcc/ctfc.h
> index 41e1169f271..e5967f590f9 100644
> --- a/gcc/ctfc.h
> +++ b/gcc/ctfc.h
> @@ -465,4 +465,6 @@ extern void btf_mark_type_used (tree);
>  extern int ctfc_get_dtd_srcloc (ctf_dtdef_ref, ctf_srcloc_ref);
>  extern int ctfc_get_dvd_srcloc (ctf_dvdef_ref, ctf_srcloc_ref);
>  
> +extern uint32_t btf_dtd_kind (ctf_dtdef_ref dtd);
> +
>  #endif /* GCC_CTFC_H */
> diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-const.c b/gcc/testsuite/gcc.target/bpf/core-attr-const.c
> new file mode 100644
> index 00000000000..da6113a3faf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/bpf/core-attr-const.c
> @@ -0,0 +1,40 @@
> +/* Test to make sure CO-RE access relocs point to non const versions of the
> +   type.  */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -dA -gbtf -mco-re -masm=normal" } */
> +
> +struct S {
> +  int a;
> +  int b;
> +  int c;
> +} __attribute__((preserve_access_index));
> +
> +void
> +func (struct S * s)
> +{
> +  int *x;
> +  int *y;
> +  int *z;
> +  struct S tmp;
> +  const struct S *cs = s;
> +  volatile struct S *vs = s;
> +
> +  /* 0:2 */
> +  x = &(s->c);
> +
> +  /* 0:1 */
> +  y = (int *) &(cs->b);
> +
> +  /* 0:1 */
> +  z = (int *) &(vs->b);
> +
> +  *x = 4;
> +  *y = 4;
> +  *z = 4;
> +}
> +
> +/* Both const and non const struct type should have the same bpfcr_type. */
> +/* { dg-final { scan-assembler-times "0x1\t# bpfcr_type \\(struct S\\)" 1 } } */
> +/* { dg-final { scan-assembler-times "0x1\t# bpfcr_type \\(const struct S\\)" 1 } } */
> +/* { dg-final { scan-assembler-times "0x1\t# bpfcr_type \\(volatile struct S\\)" 1 } } */

The issue with these checks is that you are checking the exact BTF ID,
which is, in theory, not guaranteed to remain consistent between
different runs of the compiler. Even though in practice it does for now,
this could be broken by completely unrelated changes in the
DWARF-CTF-BTF pipeline.

But, I also understand that the purpose is to make sure these three
relocation records point to exactly the same BTF type. And currently we
do not have a good way to check that.

In addition, it is misleading that the asm comments refer to the
qualified types, which no longer match the actual BTF ID in the record.
I see that this is coming from btfext_out.cc:output_btfext_core_sections
which does the pretty-print from the original type expression rather
than the BTF ID that is used.

I wonder if it would instead make sense to use the btf_asm_type_ref
function from btfout.cc to asm the type ref (and the comment), which
will do so based on the type ID rather than the original type
expression.  Then for this test we can take a similar approach to the
BTF tests: drop the check on the concrete type ID and only check the
comment to see that the three bpfcr_type are the same, i.e.

  { dg-final { scan-assembler-times " bpfcr_type \\(struct S\\)" 3 } }

This might impact the other BTF.ext tests too, but it should improve our
testing in the long run. In any case we can address this in a future patch.

WDYT?
diff mbox series

Patch

diff --git a/gcc/btfout.cc b/gcc/btfout.cc
index 8b91bde8798..24f62ec1a52 100644
--- a/gcc/btfout.cc
+++ b/gcc/btfout.cc
@@ -167,7 +167,7 @@  get_btf_kind (uint32_t ctf_kind)
 
 /* Convenience wrapper around get_btf_kind for the common case.  */
 
-static uint32_t
+uint32_t
 btf_dtd_kind (ctf_dtdef_ref dtd)
 {
   if (!dtd)
diff --git a/gcc/config/bpf/btfext-out.cc b/gcc/config/bpf/btfext-out.cc
index 095c35b894b..30266a2d384 100644
--- a/gcc/config/bpf/btfext-out.cc
+++ b/gcc/config/bpf/btfext-out.cc
@@ -320,6 +320,13 @@  bpf_core_reloc_add (const tree type, const char * section_name,
   ctf_container_ref ctfc = ctf_get_tu_ctfc ();
   ctf_dtdef_ref dtd = ctf_lookup_tree_type (ctfc, type);
 
+  /* Make sure CO-RE type is never the const or volatile version.  */
+  if ((btf_dtd_kind (dtd) == BTF_KIND_CONST
+       || btf_dtd_kind (dtd) == BTF_KIND_VOLATILE)
+      && kind >= BPF_RELO_FIELD_BYTE_OFFSET
+      && kind <= BPF_RELO_FIELD_RSHIFT_U64)
+    dtd = dtd->ref_type;
+
   /* Buffer the access string in the auxiliary strtab.  */
   bpfcr->bpfcr_astr_off = 0;
   gcc_assert (accessor != NULL);
diff --git a/gcc/ctfc.h b/gcc/ctfc.h
index 41e1169f271..e5967f590f9 100644
--- a/gcc/ctfc.h
+++ b/gcc/ctfc.h
@@ -465,4 +465,6 @@  extern void btf_mark_type_used (tree);
 extern int ctfc_get_dtd_srcloc (ctf_dtdef_ref, ctf_srcloc_ref);
 extern int ctfc_get_dvd_srcloc (ctf_dvdef_ref, ctf_srcloc_ref);
 
+extern uint32_t btf_dtd_kind (ctf_dtdef_ref dtd);
+
 #endif /* GCC_CTFC_H */
diff --git a/gcc/testsuite/gcc.target/bpf/core-attr-const.c b/gcc/testsuite/gcc.target/bpf/core-attr-const.c
new file mode 100644
index 00000000000..da6113a3faf
--- /dev/null
+++ b/gcc/testsuite/gcc.target/bpf/core-attr-const.c
@@ -0,0 +1,40 @@ 
+/* Test to make sure CO-RE access relocs point to non const versions of the
+   type.  */
+
+/* { dg-do compile } */
+/* { dg-options "-O0 -dA -gbtf -mco-re -masm=normal" } */
+
+struct S {
+  int a;
+  int b;
+  int c;
+} __attribute__((preserve_access_index));
+
+void
+func (struct S * s)
+{
+  int *x;
+  int *y;
+  int *z;
+  struct S tmp;
+  const struct S *cs = s;
+  volatile struct S *vs = s;
+
+  /* 0:2 */
+  x = &(s->c);
+
+  /* 0:1 */
+  y = (int *) &(cs->b);
+
+  /* 0:1 */
+  z = (int *) &(vs->b);
+
+  *x = 4;
+  *y = 4;
+  *z = 4;
+}
+
+/* Both const and non const struct type should have the same bpfcr_type. */
+/* { dg-final { scan-assembler-times "0x1\t# bpfcr_type \\(struct S\\)" 1 } } */
+/* { dg-final { scan-assembler-times "0x1\t# bpfcr_type \\(const struct S\\)" 1 } } */
+/* { dg-final { scan-assembler-times "0x1\t# bpfcr_type \\(volatile struct S\\)" 1 } } */