* [PATCH 0/3] btf: fix BTF for extern items [PR106773]
@ 2022-12-07 20:57 David Faust
2022-12-07 20:57 ` [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773] David Faust
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: David Faust @ 2022-12-07 20:57 UTC (permalink / raw)
To: gcc-patches; +Cc: indu.bhagat, jose.marchesi
Hi,
This series fixes the issues reported in target/PR106773. I decided to
split it into three commits, as there are ultimately three distinct
issues and fixes. See each patch for details.
Tested on bpf-unknown-none and x86_64-linux-gnu, no known regressions.
OK to push?
Thanks.
David Faust (3):
btf: add 'extern' linkage for variables [PR106773]
btf: fix 'extern const void' variables [PR106773]
btf: correct generation for extern funcs [PR106773]
gcc/btfout.cc | 182 +++++++++++++-----
.../gcc.dg/debug/btf/btf-datasec-2.c | 28 +++
.../gcc.dg/debug/btf/btf-function-6.c | 19 ++
gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c | 25 +++
.../gcc.dg/debug/btf/btf-variables-4.c | 24 +++
include/btf.h | 11 +-
6 files changed, 237 insertions(+), 52 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
--
2.38.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773]
2022-12-07 20:57 [PATCH 0/3] btf: fix BTF for extern items [PR106773] David Faust
@ 2022-12-07 20:57 ` David Faust
2022-12-09 6:55 ` Indu Bhagat
2022-12-07 20:57 ` [PATCH 2/3] btf: fix 'extern const void' " David Faust
2022-12-07 20:57 ` [PATCH 3/3] btf: correct generation for extern funcs [PR106773] David Faust
2 siblings, 1 reply; 13+ messages in thread
From: David Faust @ 2022-12-07 20:57 UTC (permalink / raw)
To: gcc-patches; +Cc: indu.bhagat, jose.marchesi
Add support for the 'extern' linkage value for BTF_KIND_VAR records,
which is used for variables declared as extern in the source file.
PR target/106773
gcc/
* btfout.cc (BTF_LINKAGE_STATIC): New define.
(BTF_LINKAGE_GLOBAL): Likewise.
(BTF_LINKAGE_EXTERN): Likewise.
(btf_collect_datasec): Mark extern variables as such.
(btf_asm_varent): Accomodate 'extern' linkage.
gcc/testsuite/
* gcc.dg/debug/btf/btf-variables-4.c: New test.
include/
* btf.h (struct btf_var): Update comment to note 'extern' linkage.
---
gcc/btfout.cc | 9 ++++++-
.../gcc.dg/debug/btf/btf-variables-4.c | 24 +++++++++++++++++++
include/btf.h | 2 +-
3 files changed, 33 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
diff --git a/gcc/btfout.cc b/gcc/btfout.cc
index aef9fd70a28..a1c6266a7db 100644
--- a/gcc/btfout.cc
+++ b/gcc/btfout.cc
@@ -66,6 +66,10 @@ static char btf_info_section_label[MAX_BTF_LABEL_BYTES];
#define BTF_INVALID_TYPEID 0xFFFFFFFF
+#define BTF_LINKAGE_STATIC 0
+#define BTF_LINKAGE_GLOBAL 1
+#define BTF_LINKAGE_EXTERN 2
+
/* Mapping of CTF variables to the IDs they will be assigned when they are
converted to BTF_KIND_VAR type records. Strictly accounts for the index
from the start of the variable type entries, does not include the number
@@ -314,6 +318,9 @@ btf_collect_datasec (ctf_container_ref ctfc)
continue;
const char *section_name = node->get_section ();
+ /* Mark extern variables. */
+ if (DECL_EXTERNAL (node->decl))
+ dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
if (section_name == NULL)
{
@@ -676,7 +683,7 @@ btf_asm_varent (ctf_dvdef_ref var)
dw2_asm_output_data (4, var->dvd_name_offset, "btv_name");
dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_VAR, 0, 0), "btv_info");
dw2_asm_output_data (4, get_btf_id (var->dvd_type), "btv_type");
- dw2_asm_output_data (4, (var->dvd_visibility ? 1 : 0), "btv_linkage");
+ dw2_asm_output_data (4, var->dvd_visibility, "btv_linkage");
}
/* Asm'out a member description following a BTF_KIND_STRUCT or
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
new file mode 100644
index 00000000000..d77600bae1c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
@@ -0,0 +1,24 @@
+/* Test BTF generation for extern variables. */
+
+/* { dg-do compile } */
+/* { dg-options "-O0 -gbtf -dA" } */
+
+/* Expect 4 variables. */
+/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 4 } } */
+
+/* 2 extern, 1 global, 1 static. */
+/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*btv_linkage" 1 } } */
+/* { dg-final { scan-assembler-times "\[\t \]0x1\[\t \]+\[^\n\]*btv_linkage" 1 } } */
+/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 2 } } */
+
+extern int a;
+extern const int b;
+int c;
+static const int d = 5;
+
+int foo (int x)
+{
+ c = a + b + x;
+
+ return c + d;
+}
diff --git a/include/btf.h b/include/btf.h
index eba67f9d599..9a757ce5bc9 100644
--- a/include/btf.h
+++ b/include/btf.h
@@ -182,7 +182,7 @@ struct btf_param
information about the variable. */
struct btf_var
{
- uint32_t linkage; /* Currently only 0=static or 1=global. */
+ uint32_t linkage; /* 0=static, 1=global, 2=extern. */
};
/* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
--
2.38.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] btf: fix 'extern const void' variables [PR106773]
2022-12-07 20:57 [PATCH 0/3] btf: fix BTF for extern items [PR106773] David Faust
2022-12-07 20:57 ` [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773] David Faust
@ 2022-12-07 20:57 ` David Faust
2022-12-09 7:34 ` Indu Bhagat
2022-12-07 20:57 ` [PATCH 3/3] btf: correct generation for extern funcs [PR106773] David Faust
2 siblings, 1 reply; 13+ messages in thread
From: David Faust @ 2022-12-07 20:57 UTC (permalink / raw)
To: gcc-patches; +Cc: indu.bhagat, jose.marchesi
The eBPF loader expects to find BTF_KIND_VAR records for references to
extern const void symbols. We were mistakenly identifing these as
unsupported types, and as a result skipping emitting VAR records for
them.
In addition, the internal DWARF representation from which BTF is
produced does not generate 'const' modifier DIEs for the void type,
which meant in BTF the 'const' qualifier was dropped for 'extern const
void' variables. This patch also adds support for generating a const
void type in BTF to correct emission for these variables.
PR target/106773
gcc/
* btfout.cc (btf_collect_datasec): Correct size of void entries.
(btf_dvd_emit_preprocess_cb): Do not skip emitting variables which
refer to void types.
(btf_init_postprocess): Create 'const void' type record if needed and
adjust variables to refer to it as appropriate.
gcc/testsuite/
* gcc.dg/debug/btf/btf-pr106773.c: New test.
---
gcc/btfout.cc | 44 +++++++++++++++++--
gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c | 25 +++++++++++
2 files changed, 65 insertions(+), 4 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
diff --git a/gcc/btfout.cc b/gcc/btfout.cc
index a1c6266a7db..05f3a3f9b6e 100644
--- a/gcc/btfout.cc
+++ b/gcc/btfout.cc
@@ -354,6 +354,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
tree size = DECL_SIZE_UNIT (node->decl);
if (tree_fits_uhwi_p (size))
info.size = tree_to_uhwi (size);
+ else if (VOID_TYPE_P (TREE_TYPE (node->decl)))
+ info.size = 1;
/* Offset is left as 0 at compile time, to be filled in by loaders such
as libbpf. */
@@ -439,7 +441,7 @@ btf_dvd_emit_preprocess_cb (ctf_dvdef_ref *slot, ctf_container_ref arg_ctfc)
ctf_dvdef_ref var = (ctf_dvdef_ref) * slot;
/* Do not add variables which refer to unsupported types. */
- if (btf_removed_type_p (var->dvd_type))
+ if (!voids.contains (var->dvd_type) && btf_removed_type_p (var->dvd_type))
return 1;
arg_ctfc->ctfc_vars_list[num_vars_added] = var;
@@ -1073,15 +1075,49 @@ btf_init_postprocess (void)
{
ctf_container_ref tu_ctfc = ctf_get_tu_ctfc ();
- size_t i;
- size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
-
holes.create (0);
voids.create (0);
num_types_added = 0;
num_types_created = 0;
+ /* Workaround for 'const void' variables. These variables are sometimes used
+ in eBPF programs to address kernel symbols. DWARF does not generate const
+ qualifier on void type, so we would incorrectly emit these variables
+ without the const qualifier.
+ Unfortunately we need the TREE node to know it was const, and we need
+ to create the const modifier type (if needed) now, before making the types
+ list. So we can't avoid iterating with FOR_EACH_VARIABLE here, and then
+ again when creating the DATASEC entries. */
+ ctf_id_t constvoid_id = CTF_NULL_TYPEID;
+ varpool_node *var;
+ FOR_EACH_VARIABLE (var)
+ {
+ if (!var->decl)
+ continue;
+
+ tree type = TREE_TYPE (var->decl);
+ if (type && VOID_TYPE_P (type) && TYPE_READONLY (type))
+ {
+ dw_die_ref die = lookup_decl_die (var->decl);
+ if (die == NULL)
+ continue;
+
+ ctf_dvdef_ref dvd = ctf_dvd_lookup (tu_ctfc, die);
+ if (dvd == NULL)
+ continue;
+
+ /* Create the 'const' modifier type for void. */
+ if (constvoid_id == CTF_NULL_TYPEID)
+ constvoid_id = ctf_add_reftype (tu_ctfc, CTF_ADD_ROOT,
+ dvd->dvd_type, CTF_K_CONST, NULL);
+ dvd->dvd_type = constvoid_id;
+ }
+ }
+
+ size_t i;
+ size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
+
if (num_ctf_types)
{
init_btf_id_map (num_ctf_types + 1);
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
new file mode 100644
index 00000000000..f90fa773a4b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
@@ -0,0 +1,25 @@
+/* Test BTF generation for extern const void symbols.
+ BTF_KIND_VAR records should be emitted for such symbols if they are used,
+ as well as a corresponding entry in the appropriate DATASEC record. */
+
+/* { dg-do compile } */
+/* { dg-options "-O0 -gbtf -dA" } */
+
+/* Expect 1 variable record only for foo, with 'extern' (2) linkage. */
+/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 1 } } */
+/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 1 } } */
+
+/* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
+
+/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
+/* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
+
+extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
+extern const void bar __attribute__((weak)) __attribute__((section (".ksyms")));
+
+unsigned long func () {
+ unsigned long x = (unsigned long) &foo;
+
+ return x;
+}
+
--
2.38.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] btf: correct generation for extern funcs [PR106773]
2022-12-07 20:57 [PATCH 0/3] btf: fix BTF for extern items [PR106773] David Faust
2022-12-07 20:57 ` [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773] David Faust
2022-12-07 20:57 ` [PATCH 2/3] btf: fix 'extern const void' " David Faust
@ 2022-12-07 20:57 ` David Faust
2022-12-09 7:36 ` Indu Bhagat
2 siblings, 1 reply; 13+ messages in thread
From: David Faust @ 2022-12-07 20:57 UTC (permalink / raw)
To: gcc-patches; +Cc: indu.bhagat, jose.marchesi
The eBPF loader expects to find entries for functions declared as extern
in the corresponding BTF_KIND_DATASEC record, but we were not generating
these entries.
This patch adds support for the 'extern' linkage of function types in
BTF, and creates entries for for them BTF_KIND_DATASEC records as needed.
PR target/106773
gcc/
* btfout.cc (get_section_name): New function.
(btf_collect_datasec): Use it here. Process functions, marking them
'extern' and generating DATASEC entries for them as appropriate. Move
creation of BTF_KIND_FUNC records to here...
(btf_dtd_emit_preprocess_cb): ... from here.
gcc/testsuite/
* gcc.dg/debug/btf/btf-datasec-2.c: New test.
* gcc.dg/debug/btf/btf-function-6.c: New test.
include/
* btf.h (struct btf_var_secinfo): Update comments with notes about
extern functions.
---
gcc/btfout.cc | 129 ++++++++++++------
.../gcc.dg/debug/btf/btf-datasec-2.c | 28 ++++
.../gcc.dg/debug/btf/btf-function-6.c | 19 +++
include/btf.h | 9 +-
4 files changed, 139 insertions(+), 46 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
diff --git a/gcc/btfout.cc b/gcc/btfout.cc
index 05f3a3f9b6e..d7ead377ec5 100644
--- a/gcc/btfout.cc
+++ b/gcc/btfout.cc
@@ -294,7 +294,35 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
ds.entries.safe_push (info);
datasecs.safe_push (ds);
- num_types_created++;
+}
+
+
+/* Return the section name, as of interest to btf_collect_datasec, for the
+ given symtab node. Note that this deliberately returns NULL for objects
+ which do not go in a section btf_collect_datasec cares about. */
+static const char *
+get_section_name (symtab_node *node)
+{
+ const char *section_name = node->get_section ();
+
+ if (section_name == NULL)
+ {
+ switch (categorize_decl_for_section (node->decl, 0))
+ {
+ case SECCAT_BSS:
+ section_name = ".bss";
+ break;
+ case SECCAT_DATA:
+ section_name = ".data";
+ break;
+ case SECCAT_RODATA:
+ section_name = ".rodata";
+ break;
+ default:;
+ }
+ }
+
+ return section_name;
}
/* Construct all BTF_KIND_DATASEC records for CTFC. One such record is created
@@ -305,7 +333,60 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
static void
btf_collect_datasec (ctf_container_ref ctfc)
{
- /* See cgraph.h struct symtab_node, which varpool_node extends. */
+ cgraph_node *func;
+ FOR_EACH_FUNCTION (func)
+ {
+ dw_die_ref die = lookup_decl_die (func->decl);
+ if (die == NULL)
+ continue;
+
+ ctf_dtdef_ref dtd = ctf_dtd_lookup (ctfc, die);
+ if (dtd == NULL)
+ continue;
+
+ /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
+ also a BTF_KIND_FUNC. But the CTF container only allocates one
+ type per function, which matches closely with BTF_KIND_FUNC_PROTO.
+ For each such function, also allocate a BTF_KIND_FUNC entry.
+ These will be output later. */
+ ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
+ func_dtd->dtd_data = dtd->dtd_data;
+ func_dtd->dtd_data.ctti_type = dtd->dtd_type;
+ func_dtd->linkage = dtd->linkage;
+ func_dtd->dtd_type = num_types_added + num_types_created;
+
+ /* Only the BTF_KIND_FUNC type actually references the name. The
+ BTF_KIND_FUNC_PROTO is always anonymous. */
+ dtd->dtd_data.ctti_name = 0;
+
+ vec_safe_push (funcs, func_dtd);
+ num_types_created++;
+
+ /* Mark any 'extern' funcs and add DATASEC entries for them. */
+ if (DECL_EXTERNAL (func->decl))
+ {
+ func_dtd->linkage = BTF_LINKAGE_EXTERN;
+
+ const char *section_name = get_section_name (func);
+ /* Note: get_section_name () returns NULL for functions in text
+ section. This is intentional, since we do not want to generate
+ DATASEC entries for them. */
+ if (section_name == NULL)
+ continue;
+
+ struct btf_var_secinfo info;
+
+ /* +1 for the sentinel type not in the types map. */
+ info.type = func_dtd->dtd_type + 1;
+
+ /* Both zero at compile time. */
+ info.size = 0;
+ info.offset = 0;
+
+ btf_datasec_push_entry (ctfc, section_name, info);
+ }
+ }
+
varpool_node *node;
FOR_EACH_VARIABLE (node)
{
@@ -317,28 +398,13 @@ btf_collect_datasec (ctf_container_ref ctfc)
if (dvd == NULL)
continue;
- const char *section_name = node->get_section ();
/* Mark extern variables. */
if (DECL_EXTERNAL (node->decl))
dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
+ const char *section_name = get_section_name (node);
if (section_name == NULL)
- {
- switch (categorize_decl_for_section (node->decl, 0))
- {
- case SECCAT_BSS:
- section_name = ".bss";
- break;
- case SECCAT_DATA:
- section_name = ".data";
- break;
- case SECCAT_RODATA:
- section_name = ".rodata";
- break;
- default:
- continue;
- }
- }
+ continue;
struct btf_var_secinfo info;
@@ -363,6 +429,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
btf_datasec_push_entry (ctfc, section_name, info);
}
+
+ num_types_created += datasecs.length ();
}
/* Return true if the type ID is that of a type which will not be emitted (for
@@ -461,29 +529,6 @@ btf_dtd_emit_preprocess_cb (ctf_container_ref ctfc, ctf_dtdef_ref dtd)
if (!btf_emit_id_p (dtd->dtd_type))
return;
- uint32_t btf_kind
- = get_btf_kind (CTF_V2_INFO_KIND (dtd->dtd_data.ctti_info));
-
- if (btf_kind == BTF_KIND_FUNC_PROTO)
- {
- /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
- also a BTF_KIND_FUNC. But the CTF container only allocates one
- type per function, which matches closely with BTF_KIND_FUNC_PROTO.
- For each such function, also allocate a BTF_KIND_FUNC entry.
- These will be output later. */
- ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
- func_dtd->dtd_data = dtd->dtd_data;
- func_dtd->dtd_data.ctti_type = dtd->dtd_type;
- func_dtd->linkage = dtd->linkage;
-
- vec_safe_push (funcs, func_dtd);
- num_types_created++;
-
- /* Only the BTF_KIND_FUNC type actually references the name. The
- BTF_KIND_FUNC_PROTO is always anonymous. */
- dtd->dtd_data.ctti_name = 0;
- }
-
ctfc->ctfc_num_vlen_bytes += btf_calc_num_vbytes (dtd);
}
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
new file mode 100644
index 00000000000..f4b298cf019
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
@@ -0,0 +1,28 @@
+/* Test BTF generation of DATASEC records for extern functions.
+
+ Only functions declared extern should have entries in DATASEC records. */
+
+/* { dg-do compile } */
+/* { dg-options "-O0 -gbtf -dA" } */
+
+/* Expect one DATASEC with vlen=1 (.foo_sec) and one with vlen=2 (.bar_sec) */
+/* { dg-final { scan-assembler-times "0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
+/* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
+
+/* Function entries should have offset and size of 0 at compile time. */
+/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
+/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
+
+extern int foo (int a) __attribute__((section(".foo_sec")));
+
+
+extern int bar (int b) __attribute__((section(".bar_sec")));
+extern void chacha (void) __attribute__((section(".bar_sec")));
+
+__attribute__((section(".foo_sec")))
+void baz (int *x)
+{
+ chacha ();
+
+ *x = foo (bar (*x));
+}
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
new file mode 100644
index 00000000000..48a946ab14b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
@@ -0,0 +1,19 @@
+/* Test BTF extern linkage for functions.
+
+ We expect to see one BTF_KIND_FUNC type with global linkage (foo), and
+ one BTF_KIND_FUNC type with extern linkage (extfunc). */
+
+/* { dg-do compile } */
+/* { dg-options "-O0 -gbtf -dA" } */
+
+/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=2" 1 } } */
+/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=1" 1 } } */
+
+extern int extfunc(int a, int b);
+
+int foo (int x) {
+
+ int y = extfunc (x, x+1);
+
+ return y;
+}
diff --git a/include/btf.h b/include/btf.h
index 9a757ce5bc9..f41ea94b75f 100644
--- a/include/btf.h
+++ b/include/btf.h
@@ -186,12 +186,13 @@ struct btf_var
};
/* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
- which describe all BTF_KIND_VAR types contained in the section. */
+ which describe all BTF_KIND_VAR or extern BTF_KIND_FUNC types contained
+ in the section. */
struct btf_var_secinfo
{
- uint32_t type; /* Type of variable. */
- uint32_t offset; /* In-section offset of variable (in bytes). */
- uint32_t size; /* Size (in bytes) of variable. */
+ uint32_t type; /* Type of BTF_KIND_VAR or BTF_KIND_FUNC item. */
+ uint32_t offset; /* In-section offset (in bytes) of item. */
+ uint32_t size; /* Size (in bytes) of item. */
};
/* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries,
--
2.38.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773]
2022-12-07 20:57 ` [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773] David Faust
@ 2022-12-09 6:55 ` Indu Bhagat
2022-12-12 20:47 ` David Faust
0 siblings, 1 reply; 13+ messages in thread
From: Indu Bhagat @ 2022-12-09 6:55 UTC (permalink / raw)
To: David Faust, gcc-patches; +Cc: jose.marchesi
Hi David,
On 12/7/22 12:57, David Faust wrote:
> Add support for the 'extern' linkage value for BTF_KIND_VAR records,
> which is used for variables declared as extern in the source file.
>
> PR target/106773
>
> gcc/
>
> * btfout.cc (BTF_LINKAGE_STATIC): New define.
> (BTF_LINKAGE_GLOBAL): Likewise.
> (BTF_LINKAGE_EXTERN): Likewise.
> (btf_collect_datasec): Mark extern variables as such.
> (btf_asm_varent): Accomodate 'extern' linkage.
>
> gcc/testsuite/
>
> * gcc.dg/debug/btf/btf-variables-4.c: New test.
>
> include/
>
> * btf.h (struct btf_var): Update comment to note 'extern' linkage.
> ---
> gcc/btfout.cc | 9 ++++++-
> .../gcc.dg/debug/btf/btf-variables-4.c | 24 +++++++++++++++++++
> include/btf.h | 2 +-
> 3 files changed, 33 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index aef9fd70a28..a1c6266a7db 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -66,6 +66,10 @@ static char btf_info_section_label[MAX_BTF_LABEL_BYTES];
>
> #define BTF_INVALID_TYPEID 0xFFFFFFFF
>
> +#define BTF_LINKAGE_STATIC 0
> +#define BTF_LINKAGE_GLOBAL 1
> +#define BTF_LINKAGE_EXTERN 2
> +
I was about to suggest to rename these to use the same name as used in
the kernel btf.h. What is used there is:
BTF_VAR_STATIC = 0,
BTF_VAR_GLOBAL_ALLOCATED = 1,
BTF_VAR_GLOBAL_EXTERN = 2,
But after looking at the Patch 3/3, I see you reuse these definitions
for functions as well. I just find the names confusing on the first look
- "BTF_LINKAGE_STATIC".
Naming aside, what do you think about adding the defines to
include/btf.h instead ?
> /* Mapping of CTF variables to the IDs they will be assigned when they are
> converted to BTF_KIND_VAR type records. Strictly accounts for the index
> from the start of the variable type entries, does not include the number
> @@ -314,6 +318,9 @@ btf_collect_datasec (ctf_container_ref ctfc)
> continue;
>
> const char *section_name = node->get_section ();
> + /* Mark extern variables. */
> + if (DECL_EXTERNAL (node->decl))
> + dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>
This made me think about the following case.
extern const char a[];
const char a[] = "foo";
What is the expected BTF for this? Since BTF can differentiate between
the non-defining extern variable declaration, I expected to see two
variables with different "linkage". At this time I see, two variables
with global linkage but different types:
.long 0xe000000 # btv_info
.long 0x4 # btv_type
.long 0x1 # btv_linkage
.long 0x1f # btv_name
.long 0xe000000 # btv_info
.long 0x7 # btv_type
.long 0x1 # btv_linkage
.long 0x60 # btt_name
> if (section_name == NULL)
> {
> @@ -676,7 +683,7 @@ btf_asm_varent (ctf_dvdef_ref var)
> dw2_asm_output_data (4, var->dvd_name_offset, "btv_name");
> dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_VAR, 0, 0), "btv_info");
> dw2_asm_output_data (4, get_btf_id (var->dvd_type), "btv_type");
> - dw2_asm_output_data (4, (var->dvd_visibility ? 1 : 0), "btv_linkage");
> + dw2_asm_output_data (4, var->dvd_visibility, "btv_linkage");
> }
>
> /* Asm'out a member description following a BTF_KIND_STRUCT or
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
> new file mode 100644
> index 00000000000..d77600bae1c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
> @@ -0,0 +1,24 @@
> +/* Test BTF generation for extern variables. */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* Expect 4 variables. */
> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 4 } } */
> +
> +/* 2 extern, 1 global, 1 static. */
> +/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*btv_linkage" 1 } } */
> +/* { dg-final { scan-assembler-times "\[\t \]0x1\[\t \]+\[^\n\]*btv_linkage" 1 } } */
> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 2 } } */
> +
> +extern int a;
> +extern const int b;
> +int c;
> +static const int d = 5;
> +
> +int foo (int x)
> +{
> + c = a + b + x;
> +
> + return c + d;
> +}
> diff --git a/include/btf.h b/include/btf.h
> index eba67f9d599..9a757ce5bc9 100644
> --- a/include/btf.h
> +++ b/include/btf.h
> @@ -182,7 +182,7 @@ struct btf_param
> information about the variable. */
> struct btf_var
> {
> - uint32_t linkage; /* Currently only 0=static or 1=global. */
> + uint32_t linkage; /* 0=static, 1=global, 2=extern. */
> };
>
> /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] btf: fix 'extern const void' variables [PR106773]
2022-12-07 20:57 ` [PATCH 2/3] btf: fix 'extern const void' " David Faust
@ 2022-12-09 7:34 ` Indu Bhagat
2022-12-12 20:59 ` David Faust
0 siblings, 1 reply; 13+ messages in thread
From: Indu Bhagat @ 2022-12-09 7:34 UTC (permalink / raw)
To: David Faust, gcc-patches; +Cc: jose.marchesi
Looks OK to me overall. Minor comments below.
Thanks
On 12/7/22 12:57, David Faust wrote:
> The eBPF loader expects to find BTF_KIND_VAR records for references to
> extern const void symbols. We were mistakenly identifing these as
> unsupported types, and as a result skipping emitting VAR records for
> them.
>
> In addition, the internal DWARF representation from which BTF is
> produced does not generate 'const' modifier DIEs for the void type,
> which meant in BTF the 'const' qualifier was dropped for 'extern const
> void' variables. This patch also adds support for generating a const
> void type in BTF to correct emission for these variables.
>
> PR target/106773
>
> gcc/
>
> * btfout.cc (btf_collect_datasec): Correct size of void entries.
> (btf_dvd_emit_preprocess_cb): Do not skip emitting variables which
> refer to void types.
> (btf_init_postprocess): Create 'const void' type record if needed and
> adjust variables to refer to it as appropriate.
>
> gcc/testsuite/
>
> * gcc.dg/debug/btf/btf-pr106773.c: New test.
> ---
> gcc/btfout.cc | 44 +++++++++++++++++--
> gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c | 25 +++++++++++
> 2 files changed, 65 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index a1c6266a7db..05f3a3f9b6e 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -354,6 +354,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
> tree size = DECL_SIZE_UNIT (node->decl);
> if (tree_fits_uhwi_p (size))
> info.size = tree_to_uhwi (size);
> + else if (VOID_TYPE_P (TREE_TYPE (node->decl)))
> + info.size = 1;
>
> /* Offset is left as 0 at compile time, to be filled in by loaders such
> as libbpf. */
> @@ -439,7 +441,7 @@ btf_dvd_emit_preprocess_cb (ctf_dvdef_ref *slot, ctf_container_ref arg_ctfc)
> ctf_dvdef_ref var = (ctf_dvdef_ref) * slot;
>
> /* Do not add variables which refer to unsupported types. */
> - if (btf_removed_type_p (var->dvd_type))
> + if (!voids.contains (var->dvd_type) && btf_removed_type_p (var->dvd_type))
> return 1;
>
> arg_ctfc->ctfc_vars_list[num_vars_added] = var;
> @@ -1073,15 +1075,49 @@ btf_init_postprocess (void)
> {
> ctf_container_ref tu_ctfc = ctf_get_tu_ctfc ();
>
> - size_t i;
> - size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
> -
> holes.create (0);
> voids.create (0);
>
> num_types_added = 0;
> num_types_created = 0;
>
> + /* Workaround for 'const void' variables. These variables are sometimes used
> + in eBPF programs to address kernel symbols. DWARF does not generate const
> + qualifier on void type, so we would incorrectly emit these variables
> + without the const qualifier.
> + Unfortunately we need the TREE node to know it was const, and we need
> + to create the const modifier type (if needed) now, before making the types
> + list. So we can't avoid iterating with FOR_EACH_VARIABLE here, and then
> + again when creating the DATASEC entries. */
"Dot, space, space, new sentence." in 3 places.
> + ctf_id_t constvoid_id = CTF_NULL_TYPEID;
> + varpool_node *var;
> + FOR_EACH_VARIABLE (var)
> + {
> + if (!var->decl)
> + continue;
> +
> + tree type = TREE_TYPE (var->decl);
> + if (type && VOID_TYPE_P (type) && TYPE_READONLY (type))
> + {
> + dw_die_ref die = lookup_decl_die (var->decl);
> + if (die == NULL)
> + continue;
> +
> + ctf_dvdef_ref dvd = ctf_dvd_lookup (tu_ctfc, die);
> + if (dvd == NULL)
> + continue;
> +
> + /* Create the 'const' modifier type for void. */
> + if (constvoid_id == CTF_NULL_TYPEID)
> + constvoid_id = ctf_add_reftype (tu_ctfc, CTF_ADD_ROOT,
> + dvd->dvd_type, CTF_K_CONST, NULL);
No de-duplication of the const void type. I assume libbpf will take
care of this eventually.
> + dvd->dvd_type = constvoid_id;
> + }
> + }
> +
> + size_t i;
> + size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
> +
> if (num_ctf_types)
> {
> init_btf_id_map (num_ctf_types + 1);
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> new file mode 100644
> index 00000000000..f90fa773a4b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> @@ -0,0 +1,25 @@
> +/* Test BTF generation for extern const void symbols.
> + BTF_KIND_VAR records should be emitted for such symbols if they are used,
> + as well as a corresponding entry in the appropriate DATASEC record. */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* Expect 1 variable record only for foo, with 'extern' (2) linkage. */
> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 1 } } */
> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 1 } } */
> +
> +/* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
> +
> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
> +/* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
> +
> +extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
> +extern const void bar __attribute__((weak)) __attribute__((section (".ksyms")));
> +
> +unsigned long func () {
> + unsigned long x = (unsigned long) &foo;
> +
> + return x;
> +}
> +
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] btf: correct generation for extern funcs [PR106773]
2022-12-07 20:57 ` [PATCH 3/3] btf: correct generation for extern funcs [PR106773] David Faust
@ 2022-12-09 7:36 ` Indu Bhagat
2022-12-12 20:31 ` David Faust
0 siblings, 1 reply; 13+ messages in thread
From: Indu Bhagat @ 2022-12-09 7:36 UTC (permalink / raw)
To: David Faust, gcc-patches; +Cc: jose.marchesi
On 12/7/22 12:57, David Faust wrote:
> The eBPF loader expects to find entries for functions declared as extern
> in the corresponding BTF_KIND_DATASEC record, but we were not generating
> these entries.
>
> This patch adds support for the 'extern' linkage of function types in
> BTF, and creates entries for for them BTF_KIND_DATASEC records as needed.
>
> PR target/106773
>
> gcc/
>
> * btfout.cc (get_section_name): New function.
> (btf_collect_datasec): Use it here. Process functions, marking them
> 'extern' and generating DATASEC entries for them as appropriate. Move
> creation of BTF_KIND_FUNC records to here...
> (btf_dtd_emit_preprocess_cb): ... from here.
>
> gcc/testsuite/
>
> * gcc.dg/debug/btf/btf-datasec-2.c: New test.
> * gcc.dg/debug/btf/btf-function-6.c: New test.
>
> include/
>
> * btf.h (struct btf_var_secinfo): Update comments with notes about
> extern functions.
> ---
> gcc/btfout.cc | 129 ++++++++++++------
> .../gcc.dg/debug/btf/btf-datasec-2.c | 28 ++++
> .../gcc.dg/debug/btf/btf-function-6.c | 19 +++
> include/btf.h | 9 +-
> 4 files changed, 139 insertions(+), 46 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index 05f3a3f9b6e..d7ead377ec5 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -294,7 +294,35 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
> ds.entries.safe_push (info);
>
> datasecs.safe_push (ds);
> - num_types_created++;
> +}
> +
> +
> +/* Return the section name, as of interest to btf_collect_datasec, for the
> + given symtab node. Note that this deliberately returns NULL for objects
> + which do not go in a section btf_collect_datasec cares about. */
"Dot, space, space, new sentence."
> +static const char *
> +get_section_name (symtab_node *node)
> +{
> + const char *section_name = node->get_section ();
> +
> + if (section_name == NULL)
> + {
> + switch (categorize_decl_for_section (node->decl, 0))
> + {
> + case SECCAT_BSS:
> + section_name = ".bss";
> + break;
> + case SECCAT_DATA:
> + section_name = ".data";
> + break;
> + case SECCAT_RODATA:
> + section_name = ".rodata";
> + break;
> + default:;
> + }
> + }
> +
> + return section_name;
> }
>
> /* Construct all BTF_KIND_DATASEC records for CTFC. One such record is created
> @@ -305,7 +333,60 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
> static void
> btf_collect_datasec (ctf_container_ref ctfc)
> {
> - /* See cgraph.h struct symtab_node, which varpool_node extends. */
> + cgraph_node *func;
> + FOR_EACH_FUNCTION (func)
> + {
> + dw_die_ref die = lookup_decl_die (func->decl);
> + if (die == NULL)
> + continue;
> +
> + ctf_dtdef_ref dtd = ctf_dtd_lookup (ctfc, die);
> + if (dtd == NULL)
> + continue;
> +
> + /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
> + also a BTF_KIND_FUNC. But the CTF container only allocates one
> + type per function, which matches closely with BTF_KIND_FUNC_PROTO.
> + For each such function, also allocate a BTF_KIND_FUNC entry.
> + These will be output later. */
"Dot, space, space, new sentence."
> + ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
> + func_dtd->dtd_data = dtd->dtd_data;
> + func_dtd->dtd_data.ctti_type = dtd->dtd_type;
> + func_dtd->linkage = dtd->linkage;
> + func_dtd->dtd_type = num_types_added + num_types_created;
> +
> + /* Only the BTF_KIND_FUNC type actually references the name. The
> + BTF_KIND_FUNC_PROTO is always anonymous. */
> + dtd->dtd_data.ctti_name = 0;
> +
> + vec_safe_push (funcs, func_dtd);
> + num_types_created++;
> +
> + /* Mark any 'extern' funcs and add DATASEC entries for them. */
> + if (DECL_EXTERNAL (func->decl))
> + {
> + func_dtd->linkage = BTF_LINKAGE_EXTERN;
> +
What is the expected BTF when both decl and definition are present:
extern int extfunc(int x);
int extfunc (int x) {
int y = foo ();
return y;
}
> + const char *section_name = get_section_name (func);
> + /* Note: get_section_name () returns NULL for functions in text
> + section. This is intentional, since we do not want to generate
> + DATASEC entries for them. */
"Dot, space, space, new sentence."
> + if (section_name == NULL)
> + continue;
> +
> + struct btf_var_secinfo info;
> +
> + /* +1 for the sentinel type not in the types map. */
> + info.type = func_dtd->dtd_type + 1;
> +
> + /* Both zero at compile time. */
> + info.size = 0;
> + info.offset = 0;
> +
> + btf_datasec_push_entry (ctfc, section_name, info);
> + }
> + }
> +
> varpool_node *node;
> FOR_EACH_VARIABLE (node)
> {
> @@ -317,28 +398,13 @@ btf_collect_datasec (ctf_container_ref ctfc)
> if (dvd == NULL)
> continue;
>
> - const char *section_name = node->get_section ();
> /* Mark extern variables. */
> if (DECL_EXTERNAL (node->decl))
> dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>
> + const char *section_name = get_section_name (node);
> if (section_name == NULL)
> - {
> - switch (categorize_decl_for_section (node->decl, 0))
> - {
> - case SECCAT_BSS:
> - section_name = ".bss";
> - break;
> - case SECCAT_DATA:
> - section_name = ".data";
> - break;
> - case SECCAT_RODATA:
> - section_name = ".rodata";
> - break;
> - default:
> - continue;
> - }
> - }
> + continue;
>
> struct btf_var_secinfo info;
>
> @@ -363,6 +429,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>
> btf_datasec_push_entry (ctfc, section_name, info);
> }
> +
> + num_types_created += datasecs.length ();
> }
>
> /* Return true if the type ID is that of a type which will not be emitted (for
> @@ -461,29 +529,6 @@ btf_dtd_emit_preprocess_cb (ctf_container_ref ctfc, ctf_dtdef_ref dtd)
> if (!btf_emit_id_p (dtd->dtd_type))
> return;
>
> - uint32_t btf_kind
> - = get_btf_kind (CTF_V2_INFO_KIND (dtd->dtd_data.ctti_info));
> -
> - if (btf_kind == BTF_KIND_FUNC_PROTO)
> - {
> - /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
> - also a BTF_KIND_FUNC. But the CTF container only allocates one
> - type per function, which matches closely with BTF_KIND_FUNC_PROTO.
> - For each such function, also allocate a BTF_KIND_FUNC entry.
> - These will be output later. */
> - ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
> - func_dtd->dtd_data = dtd->dtd_data;
> - func_dtd->dtd_data.ctti_type = dtd->dtd_type;
> - func_dtd->linkage = dtd->linkage;
> -
> - vec_safe_push (funcs, func_dtd);
> - num_types_created++;
> -
> - /* Only the BTF_KIND_FUNC type actually references the name. The
> - BTF_KIND_FUNC_PROTO is always anonymous. */
> - dtd->dtd_data.ctti_name = 0;
> - }
> -
> ctfc->ctfc_num_vlen_bytes += btf_calc_num_vbytes (dtd);
> }
>
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
> new file mode 100644
> index 00000000000..f4b298cf019
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
> @@ -0,0 +1,28 @@
> +/* Test BTF generation of DATASEC records for extern functions.
> +
> + Only functions declared extern should have entries in DATASEC records. */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* Expect one DATASEC with vlen=1 (.foo_sec) and one with vlen=2 (.bar_sec) */
> +/* { dg-final { scan-assembler-times "0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
> +/* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
> +
> +/* Function entries should have offset and size of 0 at compile time. */
> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
> +
> +extern int foo (int a) __attribute__((section(".foo_sec")));
> +
> +
> +extern int bar (int b) __attribute__((section(".bar_sec")));
> +extern void chacha (void) __attribute__((section(".bar_sec")));
> +
> +__attribute__((section(".foo_sec")))
> +void baz (int *x)
> +{
> + chacha ();
> +
> + *x = foo (bar (*x));
> +}
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
> new file mode 100644
> index 00000000000..48a946ab14b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
> @@ -0,0 +1,19 @@
> +/* Test BTF extern linkage for functions.
> +
> + We expect to see one BTF_KIND_FUNC type with global linkage (foo), and
> + one BTF_KIND_FUNC type with extern linkage (extfunc). */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=2" 1 } } */
> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=1" 1 } } */
> +
> +extern int extfunc(int a, int b);
> +
> +int foo (int x) {
> +
> + int y = extfunc (x, x+1);
> +
> + return y;
> +}
> diff --git a/include/btf.h b/include/btf.h
> index 9a757ce5bc9..f41ea94b75f 100644
> --- a/include/btf.h
> +++ b/include/btf.h
> @@ -186,12 +186,13 @@ struct btf_var
> };
>
> /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
> - which describe all BTF_KIND_VAR types contained in the section. */
> + which describe all BTF_KIND_VAR or extern BTF_KIND_FUNC types contained
> + in the section. */
> struct btf_var_secinfo
> {
> - uint32_t type; /* Type of variable. */
> - uint32_t offset; /* In-section offset of variable (in bytes). */
> - uint32_t size; /* Size (in bytes) of variable. */
> + uint32_t type; /* Type of BTF_KIND_VAR or BTF_KIND_FUNC item. */
> + uint32_t offset; /* In-section offset (in bytes) of item. */
> + uint32_t size; /* Size (in bytes) of item. */
> };
>
> /* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries,
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] btf: correct generation for extern funcs [PR106773]
2022-12-09 7:36 ` Indu Bhagat
@ 2022-12-12 20:31 ` David Faust
2022-12-13 6:12 ` Indu Bhagat
0 siblings, 1 reply; 13+ messages in thread
From: David Faust @ 2022-12-12 20:31 UTC (permalink / raw)
To: Indu Bhagat; +Cc: jose.marchesi, gcc-patches
On 12/8/22 23:36, Indu Bhagat wrote:
> On 12/7/22 12:57, David Faust wrote:
>> The eBPF loader expects to find entries for functions declared as extern
>> in the corresponding BTF_KIND_DATASEC record, but we were not generating
>> these entries.
>>
>> This patch adds support for the 'extern' linkage of function types in
>> BTF, and creates entries for for them BTF_KIND_DATASEC records as needed.
>>
>> PR target/106773
>>
>> gcc/
>>
>> * btfout.cc (get_section_name): New function.
>> (btf_collect_datasec): Use it here. Process functions, marking them
>> 'extern' and generating DATASEC entries for them as appropriate. Move
>> creation of BTF_KIND_FUNC records to here...
>> (btf_dtd_emit_preprocess_cb): ... from here.
>>
>> gcc/testsuite/
>>
>> * gcc.dg/debug/btf/btf-datasec-2.c: New test.
>> * gcc.dg/debug/btf/btf-function-6.c: New test.
>>
>> include/
>>
>> * btf.h (struct btf_var_secinfo): Update comments with notes about
>> extern functions.
>> ---
>> gcc/btfout.cc | 129 ++++++++++++------
>> .../gcc.dg/debug/btf/btf-datasec-2.c | 28 ++++
>> .../gcc.dg/debug/btf/btf-function-6.c | 19 +++
>> include/btf.h | 9 +-
>> 4 files changed, 139 insertions(+), 46 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>>
>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>> index 05f3a3f9b6e..d7ead377ec5 100644
>> --- a/gcc/btfout.cc
>> +++ b/gcc/btfout.cc
>> @@ -294,7 +294,35 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>> ds.entries.safe_push (info);
>>
>> datasecs.safe_push (ds);
>> - num_types_created++;
>> +}
>> +
>> +
>> +/* Return the section name, as of interest to btf_collect_datasec, for the
>> + given symtab node. Note that this deliberately returns NULL for objects
>> + which do not go in a section btf_collect_datasec cares about. */
>
> "Dot, space, space, new sentence."
>
>> +static const char *
>> +get_section_name (symtab_node *node)
>> +{
>> + const char *section_name = node->get_section ();
>> +
>> + if (section_name == NULL)
>> + {
>> + switch (categorize_decl_for_section (node->decl, 0))
>> + {
>> + case SECCAT_BSS:
>> + section_name = ".bss";
>> + break;
>> + case SECCAT_DATA:
>> + section_name = ".data";
>> + break;
>> + case SECCAT_RODATA:
>> + section_name = ".rodata";
>> + break;
>> + default:;
>> + }
>> + }
>> +
>> + return section_name;
>> }
>>
>> /* Construct all BTF_KIND_DATASEC records for CTFC. One such record is created
>> @@ -305,7 +333,60 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>> static void
>> btf_collect_datasec (ctf_container_ref ctfc)
>> {
>> - /* See cgraph.h struct symtab_node, which varpool_node extends. */
>> + cgraph_node *func;
>> + FOR_EACH_FUNCTION (func)
>> + {
>> + dw_die_ref die = lookup_decl_die (func->decl);
>> + if (die == NULL)
>> + continue;
>> +
>> + ctf_dtdef_ref dtd = ctf_dtd_lookup (ctfc, die);
>> + if (dtd == NULL)
>> + continue;
>> +
>> + /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
>> + also a BTF_KIND_FUNC. But the CTF container only allocates one
>> + type per function, which matches closely with BTF_KIND_FUNC_PROTO.
>> + For each such function, also allocate a BTF_KIND_FUNC entry.
>> + These will be output later. */
>
> "Dot, space, space, new sentence."
>
>> + ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
>> + func_dtd->dtd_data = dtd->dtd_data;
>> + func_dtd->dtd_data.ctti_type = dtd->dtd_type;
>> + func_dtd->linkage = dtd->linkage;
>> + func_dtd->dtd_type = num_types_added + num_types_created;
>> +
>> + /* Only the BTF_KIND_FUNC type actually references the name. The
>> + BTF_KIND_FUNC_PROTO is always anonymous. */
>> + dtd->dtd_data.ctti_name = 0;
>> +
>> + vec_safe_push (funcs, func_dtd);
>> + num_types_created++;
>> +
>> + /* Mark any 'extern' funcs and add DATASEC entries for them. */
>> + if (DECL_EXTERNAL (func->decl))
>> + {
>> + func_dtd->linkage = BTF_LINKAGE_EXTERN;
>> +
>
> What is the expected BTF when both decl and definition are present:
>
> extern int extfunc(int x);
> int extfunc (int x) {
> int y = foo ();
> return y;
> }
Using clang implementation as the reference, a single FUNC record
for "extfunc" with "global" linkage:
$ cat extfuncdef.c
extern int extfunc (int x);
int extfunc (int x) {
int y = foo ();
return y;
}
$ clang -target bpf -c -g extfuncdef.c -o extfuncdef.o
$ /usr/sbin/bpftool btf dump file extfuncdef.o
[1] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1
'(anon)' type_id=2
[2] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
[3] FUNC 'extfunc' type_id=1 linkage=global
With this patch we do the same in GCC.
>
>> + const char *section_name = get_section_name (func);
>> + /* Note: get_section_name () returns NULL for functions in text
>> + section. This is intentional, since we do not want to generate
>> + DATASEC entries for them. */
>
> "Dot, space, space, new sentence."
>
>> + if (section_name == NULL)
>> + continue;
>> +
>> + struct btf_var_secinfo info;
>> +
>> + /* +1 for the sentinel type not in the types map. */
>> + info.type = func_dtd->dtd_type + 1;
>> +
>> + /* Both zero at compile time. */
>> + info.size = 0;
>> + info.offset = 0;
>> +
>> + btf_datasec_push_entry (ctfc, section_name, info);
>> + }
>> + }
>> +
>> varpool_node *node;
>> FOR_EACH_VARIABLE (node)
>> {
>> @@ -317,28 +398,13 @@ btf_collect_datasec (ctf_container_ref ctfc)
>> if (dvd == NULL)
>> continue;
>>
>> - const char *section_name = node->get_section ();
>> /* Mark extern variables. */
>> if (DECL_EXTERNAL (node->decl))
>> dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>>
>> + const char *section_name = get_section_name (node);
>> if (section_name == NULL)
>> - {
>> - switch (categorize_decl_for_section (node->decl, 0))
>> - {
>> - case SECCAT_BSS:
>> - section_name = ".bss";
>> - break;
>> - case SECCAT_DATA:
>> - section_name = ".data";
>> - break;
>> - case SECCAT_RODATA:
>> - section_name = ".rodata";
>> - break;
>> - default:
>> - continue;
>> - }
>> - }
>> + continue;
>>
>> struct btf_var_secinfo info;
>>
>> @@ -363,6 +429,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>
>> btf_datasec_push_entry (ctfc, section_name, info);
>> }
>> +
>> + num_types_created += datasecs.length ();
>> }
>>
>> /* Return true if the type ID is that of a type which will not be emitted (for
>> @@ -461,29 +529,6 @@ btf_dtd_emit_preprocess_cb (ctf_container_ref ctfc, ctf_dtdef_ref dtd)
>> if (!btf_emit_id_p (dtd->dtd_type))
>> return;
>>
>> - uint32_t btf_kind
>> - = get_btf_kind (CTF_V2_INFO_KIND (dtd->dtd_data.ctti_info));
>> -
>> - if (btf_kind == BTF_KIND_FUNC_PROTO)
>> - {
>> - /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
>> - also a BTF_KIND_FUNC. But the CTF container only allocates one
>> - type per function, which matches closely with BTF_KIND_FUNC_PROTO.
>> - For each such function, also allocate a BTF_KIND_FUNC entry.
>> - These will be output later. */
>> - ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
>> - func_dtd->dtd_data = dtd->dtd_data;
>> - func_dtd->dtd_data.ctti_type = dtd->dtd_type;
>> - func_dtd->linkage = dtd->linkage;
>> -
>> - vec_safe_push (funcs, func_dtd);
>> - num_types_created++;
>> -
>> - /* Only the BTF_KIND_FUNC type actually references the name. The
>> - BTF_KIND_FUNC_PROTO is always anonymous. */
>> - dtd->dtd_data.ctti_name = 0;
>> - }
>> -
>> ctfc->ctfc_num_vlen_bytes += btf_calc_num_vbytes (dtd);
>> }
>>
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>> new file mode 100644
>> index 00000000000..f4b298cf019
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>> @@ -0,0 +1,28 @@
>> +/* Test BTF generation of DATASEC records for extern functions.
>> +
>> + Only functions declared extern should have entries in DATASEC records. */
>> +
>> +/* { dg-do compile } */
>> +/* { dg-options "-O0 -gbtf -dA" } */
>> +
>> +/* Expect one DATASEC with vlen=1 (.foo_sec) and one with vlen=2 (.bar_sec) */
>> +/* { dg-final { scan-assembler-times "0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
>> +/* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
>> +
>> +/* Function entries should have offset and size of 0 at compile time. */
>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
>> +
>> +extern int foo (int a) __attribute__((section(".foo_sec")));
>> +
>> +
>> +extern int bar (int b) __attribute__((section(".bar_sec")));
>> +extern void chacha (void) __attribute__((section(".bar_sec")));
>> +
>> +__attribute__((section(".foo_sec")))
>> +void baz (int *x)
>> +{
>> + chacha ();
>> +
>> + *x = foo (bar (*x));
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>> new file mode 100644
>> index 00000000000..48a946ab14b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>> @@ -0,0 +1,19 @@
>> +/* Test BTF extern linkage for functions.
>> +
>> + We expect to see one BTF_KIND_FUNC type with global linkage (foo), and
>> + one BTF_KIND_FUNC type with extern linkage (extfunc). */
>> +
>> +/* { dg-do compile } */
>> +/* { dg-options "-O0 -gbtf -dA" } */
>> +
>> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=2" 1 } } */
>> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=1" 1 } } */
>> +
>> +extern int extfunc(int a, int b);
>> +
>> +int foo (int x) {
>> +
>> + int y = extfunc (x, x+1);
>> +
>> + return y;
>> +}
>> diff --git a/include/btf.h b/include/btf.h
>> index 9a757ce5bc9..f41ea94b75f 100644
>> --- a/include/btf.h
>> +++ b/include/btf.h
>> @@ -186,12 +186,13 @@ struct btf_var
>> };
>>
>> /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
>> - which describe all BTF_KIND_VAR types contained in the section. */
>> + which describe all BTF_KIND_VAR or extern BTF_KIND_FUNC types contained
>> + in the section. */
>> struct btf_var_secinfo
>> {
>> - uint32_t type; /* Type of variable. */
>> - uint32_t offset; /* In-section offset of variable (in bytes). */
>> - uint32_t size; /* Size (in bytes) of variable. */
>> + uint32_t type; /* Type of BTF_KIND_VAR or BTF_KIND_FUNC item. */
>> + uint32_t offset; /* In-section offset (in bytes) of item. */
>> + uint32_t size; /* Size (in bytes) of item. */
>> };
>>
>> /* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries,
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773]
2022-12-09 6:55 ` Indu Bhagat
@ 2022-12-12 20:47 ` David Faust
2022-12-13 6:15 ` Indu Bhagat
0 siblings, 1 reply; 13+ messages in thread
From: David Faust @ 2022-12-12 20:47 UTC (permalink / raw)
To: Indu Bhagat; +Cc: jose.marchesi, gcc-patches
On 12/8/22 22:55, Indu Bhagat wrote:
> Hi David,
>
> On 12/7/22 12:57, David Faust wrote:
>> Add support for the 'extern' linkage value for BTF_KIND_VAR records,
>> which is used for variables declared as extern in the source file.
>>
>> PR target/106773
>>
>> gcc/
>>
>> * btfout.cc (BTF_LINKAGE_STATIC): New define.
>> (BTF_LINKAGE_GLOBAL): Likewise.
>> (BTF_LINKAGE_EXTERN): Likewise.
>> (btf_collect_datasec): Mark extern variables as such.
>> (btf_asm_varent): Accomodate 'extern' linkage.
>>
>> gcc/testsuite/
>>
>> * gcc.dg/debug/btf/btf-variables-4.c: New test.
>>
>> include/
>>
>> * btf.h (struct btf_var): Update comment to note 'extern' linkage.
>> ---
>> gcc/btfout.cc | 9 ++++++-
>> .../gcc.dg/debug/btf/btf-variables-4.c | 24 +++++++++++++++++++
>> include/btf.h | 2 +-
>> 3 files changed, 33 insertions(+), 2 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>>
>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>> index aef9fd70a28..a1c6266a7db 100644
>> --- a/gcc/btfout.cc
>> +++ b/gcc/btfout.cc
>> @@ -66,6 +66,10 @@ static char btf_info_section_label[MAX_BTF_LABEL_BYTES];
>>
>> #define BTF_INVALID_TYPEID 0xFFFFFFFF
>>
>> +#define BTF_LINKAGE_STATIC 0
>> +#define BTF_LINKAGE_GLOBAL 1
>> +#define BTF_LINKAGE_EXTERN 2
>> +
>
> I was about to suggest to rename these to use the same name as used in
> the kernel btf.h. What is used there is:
> BTF_VAR_STATIC = 0,
> BTF_VAR_GLOBAL_ALLOCATED = 1,
> BTF_VAR_GLOBAL_EXTERN = 2,
>
> But after looking at the Patch 3/3, I see you reuse these definitions
> for functions as well. I just find the names confusing on the first look
> - "BTF_LINKAGE_STATIC".
>
> Naming aside, what do you think about adding the defines to
> include/btf.h instead ?
Actually, I forgot these are defined (separately for both VARs and FUNCs)
in the kernel uapi/linux/btf.h. It would probably be best to mirror that
approach and use a separate enum for each, in include/btf.h. WDYT?
>
>> /* Mapping of CTF variables to the IDs they will be assigned when they are
>> converted to BTF_KIND_VAR type records. Strictly accounts for the index
>> from the start of the variable type entries, does not include the number
>> @@ -314,6 +318,9 @@ btf_collect_datasec (ctf_container_ref ctfc)
>> continue;
>>
>> const char *section_name = node->get_section ();
>> + /* Mark extern variables. */
>> + if (DECL_EXTERNAL (node->decl))
>> + dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>>
>
> This made me think about the following case.
>
> extern const char a[];
> const char a[] = "foo";
>
> What is the expected BTF for this? Since BTF can differentiate between
> the non-defining extern variable declaration, I expected to see two
> variables with different "linkage". At this time I see, two variables
> with global linkage but different types:
>
> .long 0xe000000 # btv_info
> .long 0x4 # btv_type
> .long 0x1 # btv_linkage
> .long 0x1f # btv_name
> .long 0xe000000 # btv_info
> .long 0x7 # btv_type
> .long 0x1 # btv_linkage
> .long 0x60 # btt_name
>
The BTF documentation in the kernel does not clarify this case.
Going off the implementation in clang as a reference, it looks like
only one VAR record is expected, with 'global' linkage:
$ cat extdef.c
extern const char a[];
const char a[] = "foo";
$ clang -target bpf -c -g extdef.c -o extdef.o
$ /usr/sbin/bpftool btf dump file extdef.o
[1] CONST '(anon)' type_id=2
[2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED
[3] ARRAY '(anon)' type_id=1 index_type_id=4 nr_elems=4
[4] INT '__ARRAY_SIZE_TYPE__' size=4 bits_offset=0 nr_bits=32 encoding=(none)
[5] VAR 'a' type_id=3, linkage=global
[6] DATASEC '.rodata' size=0 vlen=1
type_id=5 offset=0 size=4 (VAR 'a')
In GCC we have two records since we have two DIEs for "a" in the
DWARF. One has type "const char[4]" and the other has type
"const char[]", so the BTF records point to two different types
as well.
I guess we should find a way in BTF to identify this and
emit only the defining definition as clang does.
>> if (section_name == NULL)
>> {
>> @@ -676,7 +683,7 @@ btf_asm_varent (ctf_dvdef_ref var)
>> dw2_asm_output_data (4, var->dvd_name_offset, "btv_name");
>> dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_VAR, 0, 0), "btv_info");
>> dw2_asm_output_data (4, get_btf_id (var->dvd_type), "btv_type");
>> - dw2_asm_output_data (4, (var->dvd_visibility ? 1 : 0), "btv_linkage");
>> + dw2_asm_output_data (4, var->dvd_visibility, "btv_linkage");
>> }
>>
>> /* Asm'out a member description following a BTF_KIND_STRUCT or
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>> new file mode 100644
>> index 00000000000..d77600bae1c
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>> @@ -0,0 +1,24 @@
>> +/* Test BTF generation for extern variables. */
>> +
>> +/* { dg-do compile } */
>> +/* { dg-options "-O0 -gbtf -dA" } */
>> +
>> +/* Expect 4 variables. */
>> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 4 } } */
>> +
>> +/* 2 extern, 1 global, 1 static. */
>> +/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>> +/* { dg-final { scan-assembler-times "\[\t \]0x1\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 2 } } */
>> +
>> +extern int a;
>> +extern const int b;
>> +int c;
>> +static const int d = 5;
>> +
>> +int foo (int x)
>> +{
>> + c = a + b + x;
>> +
>> + return c + d;
>> +}
>> diff --git a/include/btf.h b/include/btf.h
>> index eba67f9d599..9a757ce5bc9 100644
>> --- a/include/btf.h
>> +++ b/include/btf.h
>> @@ -182,7 +182,7 @@ struct btf_param
>> information about the variable. */
>> struct btf_var
>> {
>> - uint32_t linkage; /* Currently only 0=static or 1=global. */
>> + uint32_t linkage; /* 0=static, 1=global, 2=extern. */
>> };
>>
>> /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] btf: fix 'extern const void' variables [PR106773]
2022-12-09 7:34 ` Indu Bhagat
@ 2022-12-12 20:59 ` David Faust
2022-12-13 6:11 ` Indu Bhagat
0 siblings, 1 reply; 13+ messages in thread
From: David Faust @ 2022-12-12 20:59 UTC (permalink / raw)
To: Indu Bhagat; +Cc: jose.marchesi, gcc-patches
On 12/8/22 23:34, Indu Bhagat wrote:
> Looks OK to me overall. Minor comments below.
>
> Thanks
>
> On 12/7/22 12:57, David Faust wrote:
>> The eBPF loader expects to find BTF_KIND_VAR records for references to
>> extern const void symbols. We were mistakenly identifing these as
>> unsupported types, and as a result skipping emitting VAR records for
>> them.
>>
>> In addition, the internal DWARF representation from which BTF is
>> produced does not generate 'const' modifier DIEs for the void type,
>> which meant in BTF the 'const' qualifier was dropped for 'extern const
>> void' variables. This patch also adds support for generating a const
>> void type in BTF to correct emission for these variables.
>>
>> PR target/106773
>>
>> gcc/
>>
>> * btfout.cc (btf_collect_datasec): Correct size of void entries.
>> (btf_dvd_emit_preprocess_cb): Do not skip emitting variables which
>> refer to void types.
>> (btf_init_postprocess): Create 'const void' type record if needed and
>> adjust variables to refer to it as appropriate.
>>
>> gcc/testsuite/
>>
>> * gcc.dg/debug/btf/btf-pr106773.c: New test.
>> ---
>> gcc/btfout.cc | 44 +++++++++++++++++--
>> gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c | 25 +++++++++++
>> 2 files changed, 65 insertions(+), 4 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>>
>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>> index a1c6266a7db..05f3a3f9b6e 100644
>> --- a/gcc/btfout.cc
>> +++ b/gcc/btfout.cc
>> @@ -354,6 +354,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>> tree size = DECL_SIZE_UNIT (node->decl);
>> if (tree_fits_uhwi_p (size))
>> info.size = tree_to_uhwi (size);
>> + else if (VOID_TYPE_P (TREE_TYPE (node->decl)))
>> + info.size = 1;
>>
>> /* Offset is left as 0 at compile time, to be filled in by loaders such
>> as libbpf. */
>> @@ -439,7 +441,7 @@ btf_dvd_emit_preprocess_cb (ctf_dvdef_ref *slot, ctf_container_ref arg_ctfc)
>> ctf_dvdef_ref var = (ctf_dvdef_ref) * slot;
>>
>> /* Do not add variables which refer to unsupported types. */
>> - if (btf_removed_type_p (var->dvd_type))
>> + if (!voids.contains (var->dvd_type) && btf_removed_type_p (var->dvd_type))
>> return 1;
>>
>> arg_ctfc->ctfc_vars_list[num_vars_added] = var;
>> @@ -1073,15 +1075,49 @@ btf_init_postprocess (void)
>> {
>> ctf_container_ref tu_ctfc = ctf_get_tu_ctfc ();
>>
>> - size_t i;
>> - size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
>> -
>> holes.create (0);
>> voids.create (0);
>>
>> num_types_added = 0;
>> num_types_created = 0;
>>
>> + /* Workaround for 'const void' variables. These variables are sometimes used
>> + in eBPF programs to address kernel symbols. DWARF does not generate const
>> + qualifier on void type, so we would incorrectly emit these variables
>> + without the const qualifier.
>> + Unfortunately we need the TREE node to know it was const, and we need
>> + to create the const modifier type (if needed) now, before making the types
>> + list. So we can't avoid iterating with FOR_EACH_VARIABLE here, and then
>> + again when creating the DATASEC entries. */
>
> "Dot, space, space, new sentence." in 3 places.
>
>
>> + ctf_id_t constvoid_id = CTF_NULL_TYPEID;
>> + varpool_node *var;
>> + FOR_EACH_VARIABLE (var)
>> + {
>> + if (!var->decl)
>> + continue;
>> +
>> + tree type = TREE_TYPE (var->decl);
>> + if (type && VOID_TYPE_P (type) && TYPE_READONLY (type))
>> + {
>> + dw_die_ref die = lookup_decl_die (var->decl);
>> + if (die == NULL)
>> + continue;
>> +
>> + ctf_dvdef_ref dvd = ctf_dvd_lookup (tu_ctfc, die);
>> + if (dvd == NULL)
>> + continue;
>> +
>> + /* Create the 'const' modifier type for void. */
>> + if (constvoid_id == CTF_NULL_TYPEID)
>> + constvoid_id = ctf_add_reftype (tu_ctfc, CTF_ADD_ROOT,
>> + dvd->dvd_type, CTF_K_CONST, NULL);
>
> No de-duplication of the const void type. I assume libbpf will take
> care of this eventually.
Hm, not sure I follow. Where is the duplication? The const void type is
only created once here, for the first such variable which needs it, and
reused for subsequent variables. And it does not already exist in the
CTF which we are translating into BTF.
In any case, yes libbpf can handle duplicated types. Though it would
still be good to minimize that where we can to not bloat the BTF info.
>
>> + dvd->dvd_type = constvoid_id;
>> + }
>> + }
>> +
>> + size_t i;
>> + size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
>> +
>> if (num_ctf_types)
>> {
>> init_btf_id_map (num_ctf_types + 1);
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>> new file mode 100644
>> index 00000000000..f90fa773a4b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>> @@ -0,0 +1,25 @@
>> +/* Test BTF generation for extern const void symbols.
>> + BTF_KIND_VAR records should be emitted for such symbols if they are used,
>> + as well as a corresponding entry in the appropriate DATASEC record. */
>> +
>> +/* { dg-do compile } */
>> +/* { dg-options "-O0 -gbtf -dA" } */
>> +
>> +/* Expect 1 variable record only for foo, with 'extern' (2) linkage. */
>> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 1 } } */
>> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>> +
>> +/* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
>> +
>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
>> +/* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
>> +
>> +extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
>> +extern const void bar __attribute__((weak)) __attribute__((section (".ksyms")));
>> +
>> +unsigned long func () {
>> + unsigned long x = (unsigned long) &foo;
>> +
>> + return x;
>> +}
>> +
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] btf: fix 'extern const void' variables [PR106773]
2022-12-12 20:59 ` David Faust
@ 2022-12-13 6:11 ` Indu Bhagat
0 siblings, 0 replies; 13+ messages in thread
From: Indu Bhagat @ 2022-12-13 6:11 UTC (permalink / raw)
To: David Faust; +Cc: jose.marchesi, gcc-patches
On 12/12/22 12:59, David Faust wrote:
>
>
> On 12/8/22 23:34, Indu Bhagat wrote:
>> Looks OK to me overall. Minor comments below.
>>
>> Thanks
>>
>> On 12/7/22 12:57, David Faust wrote:
>>> The eBPF loader expects to find BTF_KIND_VAR records for references to
>>> extern const void symbols. We were mistakenly identifing these as
>>> unsupported types, and as a result skipping emitting VAR records for
>>> them.
>>>
>>> In addition, the internal DWARF representation from which BTF is
>>> produced does not generate 'const' modifier DIEs for the void type,
>>> which meant in BTF the 'const' qualifier was dropped for 'extern const
>>> void' variables. This patch also adds support for generating a const
>>> void type in BTF to correct emission for these variables.
>>>
>>> PR target/106773
>>>
>>> gcc/
>>>
>>> * btfout.cc (btf_collect_datasec): Correct size of void entries.
>>> (btf_dvd_emit_preprocess_cb): Do not skip emitting variables which
>>> refer to void types.
>>> (btf_init_postprocess): Create 'const void' type record if needed and
>>> adjust variables to refer to it as appropriate.
>>>
>>> gcc/testsuite/
>>>
>>> * gcc.dg/debug/btf/btf-pr106773.c: New test.
>>> ---
>>> gcc/btfout.cc | 44 +++++++++++++++++--
>>> gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c | 25 +++++++++++
>>> 2 files changed, 65 insertions(+), 4 deletions(-)
>>> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>>>
>>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>>> index a1c6266a7db..05f3a3f9b6e 100644
>>> --- a/gcc/btfout.cc
>>> +++ b/gcc/btfout.cc
>>> @@ -354,6 +354,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>> tree size = DECL_SIZE_UNIT (node->decl);
>>> if (tree_fits_uhwi_p (size))
>>> info.size = tree_to_uhwi (size);
>>> + else if (VOID_TYPE_P (TREE_TYPE (node->decl)))
>>> + info.size = 1;
>>>
>>> /* Offset is left as 0 at compile time, to be filled in by loaders such
>>> as libbpf. */
>>> @@ -439,7 +441,7 @@ btf_dvd_emit_preprocess_cb (ctf_dvdef_ref *slot, ctf_container_ref arg_ctfc)
>>> ctf_dvdef_ref var = (ctf_dvdef_ref) * slot;
>>>
>>> /* Do not add variables which refer to unsupported types. */
>>> - if (btf_removed_type_p (var->dvd_type))
>>> + if (!voids.contains (var->dvd_type) && btf_removed_type_p (var->dvd_type))
>>> return 1;
>>>
>>> arg_ctfc->ctfc_vars_list[num_vars_added] = var;
>>> @@ -1073,15 +1075,49 @@ btf_init_postprocess (void)
>>> {
>>> ctf_container_ref tu_ctfc = ctf_get_tu_ctfc ();
>>>
>>> - size_t i;
>>> - size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
>>> -
>>> holes.create (0);
>>> voids.create (0);
>>>
>>> num_types_added = 0;
>>> num_types_created = 0;
>>>
>>> + /* Workaround for 'const void' variables. These variables are sometimes used
>>> + in eBPF programs to address kernel symbols. DWARF does not generate const
>>> + qualifier on void type, so we would incorrectly emit these variables
>>> + without the const qualifier.
>>> + Unfortunately we need the TREE node to know it was const, and we need
>>> + to create the const modifier type (if needed) now, before making the types
>>> + list. So we can't avoid iterating with FOR_EACH_VARIABLE here, and then
>>> + again when creating the DATASEC entries. */
>>
>> "Dot, space, space, new sentence." in 3 places.
>>
>>
>>> + ctf_id_t constvoid_id = CTF_NULL_TYPEID;
>>> + varpool_node *var;
>>> + FOR_EACH_VARIABLE (var)
>>> + {
>>> + if (!var->decl)
>>> + continue;
>>> +
>>> + tree type = TREE_TYPE (var->decl);
>>> + if (type && VOID_TYPE_P (type) && TYPE_READONLY (type))
>>> + {
>>> + dw_die_ref die = lookup_decl_die (var->decl);
>>> + if (die == NULL)
>>> + continue;
>>> +
>>> + ctf_dvdef_ref dvd = ctf_dvd_lookup (tu_ctfc, die);
>>> + if (dvd == NULL)
>>> + continue;
>>> +
>>> + /* Create the 'const' modifier type for void. */
>>> + if (constvoid_id == CTF_NULL_TYPEID)
>>> + constvoid_id = ctf_add_reftype (tu_ctfc, CTF_ADD_ROOT,
>>> + dvd->dvd_type, CTF_K_CONST, NULL);
>>
>> No de-duplication of the const void type. I assume libbpf will take
>> care of this eventually.
>
> Hm, not sure I follow. Where is the duplication? The const void type is
> only created once here, for the first such variable which needs it, and
> reused for subsequent variables. And it does not already exist in the
> CTF which we are translating into BTF.
>
You're right - you are reusing the const void type once generated for a
CU for each usage. My bad - I didnt follow the code properly :)
> In any case, yes libbpf can handle duplicated types. Though it would
> still be good to minimize that where we can to not bloat the BTF info.
>
>>
>>> + dvd->dvd_type = constvoid_id;
>>> + }
>>> + }
>>> +
>>> + size_t i;
>>> + size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
>>> +
>>> if (num_ctf_types)
>>> {
>>> init_btf_id_map (num_ctf_types + 1);
>>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>>> new file mode 100644
>>> index 00000000000..f90fa773a4b
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>>> @@ -0,0 +1,25 @@
>>> +/* Test BTF generation for extern const void symbols.
>>> + BTF_KIND_VAR records should be emitted for such symbols if they are used,
>>> + as well as a corresponding entry in the appropriate DATASEC record. */
>>> +
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O0 -gbtf -dA" } */
>>> +
>>> +/* Expect 1 variable record only for foo, with 'extern' (2) linkage. */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 1 } } */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>>> +
>>> +/* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
>>> +
>>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
>>> +/* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
>>> +
>>> +extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
>>> +extern const void bar __attribute__((weak)) __attribute__((section (".ksyms")));
>>> +
>>> +unsigned long func () {
>>> + unsigned long x = (unsigned long) &foo;
>>> +
>>> + return x;
>>> +}
>>> +
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] btf: correct generation for extern funcs [PR106773]
2022-12-12 20:31 ` David Faust
@ 2022-12-13 6:12 ` Indu Bhagat
0 siblings, 0 replies; 13+ messages in thread
From: Indu Bhagat @ 2022-12-13 6:12 UTC (permalink / raw)
To: David Faust; +Cc: jose.marchesi, gcc-patches
On 12/12/22 12:31, David Faust wrote:
>
>
> On 12/8/22 23:36, Indu Bhagat wrote:
>> On 12/7/22 12:57, David Faust wrote:
>>> The eBPF loader expects to find entries for functions declared as extern
>>> in the corresponding BTF_KIND_DATASEC record, but we were not generating
>>> these entries.
>>>
>>> This patch adds support for the 'extern' linkage of function types in
>>> BTF, and creates entries for for them BTF_KIND_DATASEC records as needed.
>>>
>>> PR target/106773
>>>
>>> gcc/
>>>
>>> * btfout.cc (get_section_name): New function.
>>> (btf_collect_datasec): Use it here. Process functions, marking them
>>> 'extern' and generating DATASEC entries for them as appropriate. Move
>>> creation of BTF_KIND_FUNC records to here...
>>> (btf_dtd_emit_preprocess_cb): ... from here.
>>>
>>> gcc/testsuite/
>>>
>>> * gcc.dg/debug/btf/btf-datasec-2.c: New test.
>>> * gcc.dg/debug/btf/btf-function-6.c: New test.
>>>
>>> include/
>>>
>>> * btf.h (struct btf_var_secinfo): Update comments with notes about
>>> extern functions.
>>> ---
>>> gcc/btfout.cc | 129 ++++++++++++------
>>> .../gcc.dg/debug/btf/btf-datasec-2.c | 28 ++++
>>> .../gcc.dg/debug/btf/btf-function-6.c | 19 +++
>>> include/btf.h | 9 +-
>>> 4 files changed, 139 insertions(+), 46 deletions(-)
>>> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>>> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>>>
>>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>>> index 05f3a3f9b6e..d7ead377ec5 100644
>>> --- a/gcc/btfout.cc
>>> +++ b/gcc/btfout.cc
>>> @@ -294,7 +294,35 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>>> ds.entries.safe_push (info);
>>>
>>> datasecs.safe_push (ds);
>>> - num_types_created++;
>>> +}
>>> +
>>> +
>>> +/* Return the section name, as of interest to btf_collect_datasec, for the
>>> + given symtab node. Note that this deliberately returns NULL for objects
>>> + which do not go in a section btf_collect_datasec cares about. */
>>
>> "Dot, space, space, new sentence."
>>
>>> +static const char *
>>> +get_section_name (symtab_node *node)
>>> +{
>>> + const char *section_name = node->get_section ();
>>> +
>>> + if (section_name == NULL)
>>> + {
>>> + switch (categorize_decl_for_section (node->decl, 0))
>>> + {
>>> + case SECCAT_BSS:
>>> + section_name = ".bss";
>>> + break;
>>> + case SECCAT_DATA:
>>> + section_name = ".data";
>>> + break;
>>> + case SECCAT_RODATA:
>>> + section_name = ".rodata";
>>> + break;
>>> + default:;
>>> + }
>>> + }
>>> +
>>> + return section_name;
>>> }
>>>
>>> /* Construct all BTF_KIND_DATASEC records for CTFC. One such record is created
>>> @@ -305,7 +333,60 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>>> static void
>>> btf_collect_datasec (ctf_container_ref ctfc)
>>> {
>>> - /* See cgraph.h struct symtab_node, which varpool_node extends. */
>>> + cgraph_node *func;
>>> + FOR_EACH_FUNCTION (func)
>>> + {
>>> + dw_die_ref die = lookup_decl_die (func->decl);
>>> + if (die == NULL)
>>> + continue;
>>> +
>>> + ctf_dtdef_ref dtd = ctf_dtd_lookup (ctfc, die);
>>> + if (dtd == NULL)
>>> + continue;
>>> +
>>> + /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
>>> + also a BTF_KIND_FUNC. But the CTF container only allocates one
>>> + type per function, which matches closely with BTF_KIND_FUNC_PROTO.
>>> + For each such function, also allocate a BTF_KIND_FUNC entry.
>>> + These will be output later. */
>>
>> "Dot, space, space, new sentence."
>>
>>> + ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
>>> + func_dtd->dtd_data = dtd->dtd_data;
>>> + func_dtd->dtd_data.ctti_type = dtd->dtd_type;
>>> + func_dtd->linkage = dtd->linkage;
>>> + func_dtd->dtd_type = num_types_added + num_types_created;
>>> +
>>> + /* Only the BTF_KIND_FUNC type actually references the name. The
>>> + BTF_KIND_FUNC_PROTO is always anonymous. */
>>> + dtd->dtd_data.ctti_name = 0;
>>> +
>>> + vec_safe_push (funcs, func_dtd);
>>> + num_types_created++;
>>> +
>>> + /* Mark any 'extern' funcs and add DATASEC entries for them. */
>>> + if (DECL_EXTERNAL (func->decl))
>>> + {
>>> + func_dtd->linkage = BTF_LINKAGE_EXTERN;
>>> +
>>
>> What is the expected BTF when both decl and definition are present:
>>
>> extern int extfunc(int x);
>> int extfunc (int x) {
>> int y = foo ();
>> return y;
>> }
>
> Using clang implementation as the reference, a single FUNC record
> for "extfunc" with "global" linkage:
>
> $ cat extfuncdef.c
> extern int extfunc (int x);
> int extfunc (int x) {
> int y = foo ();
> return y;
> }
>
> $ clang -target bpf -c -g extfuncdef.c -o extfuncdef.o
>
> $ /usr/sbin/bpftool btf dump file extfuncdef.o
> [1] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1
> '(anon)' type_id=2
> [2] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> [3] FUNC 'extfunc' type_id=1 linkage=global
>
> With this patch we do the same in GCC.
>
OK. Thanks for confirming.
>>
>>> + const char *section_name = get_section_name (func);
>>> + /* Note: get_section_name () returns NULL for functions in text
>>> + section. This is intentional, since we do not want to generate
>>> + DATASEC entries for them. */
>>
>> "Dot, space, space, new sentence."
>>
>>> + if (section_name == NULL)
>>> + continue;
>>> +
>>> + struct btf_var_secinfo info;
>>> +
>>> + /* +1 for the sentinel type not in the types map. */
>>> + info.type = func_dtd->dtd_type + 1;
>>> +
>>> + /* Both zero at compile time. */
>>> + info.size = 0;
>>> + info.offset = 0;
>>> +
>>> + btf_datasec_push_entry (ctfc, section_name, info);
>>> + }
>>> + }
>>> +
>>> varpool_node *node;
>>> FOR_EACH_VARIABLE (node)
>>> {
>>> @@ -317,28 +398,13 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>> if (dvd == NULL)
>>> continue;
>>>
>>> - const char *section_name = node->get_section ();
>>> /* Mark extern variables. */
>>> if (DECL_EXTERNAL (node->decl))
>>> dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>>>
>>> + const char *section_name = get_section_name (node);
>>> if (section_name == NULL)
>>> - {
>>> - switch (categorize_decl_for_section (node->decl, 0))
>>> - {
>>> - case SECCAT_BSS:
>>> - section_name = ".bss";
>>> - break;
>>> - case SECCAT_DATA:
>>> - section_name = ".data";
>>> - break;
>>> - case SECCAT_RODATA:
>>> - section_name = ".rodata";
>>> - break;
>>> - default:
>>> - continue;
>>> - }
>>> - }
>>> + continue;
>>>
>>> struct btf_var_secinfo info;
>>>
>>> @@ -363,6 +429,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>>
>>> btf_datasec_push_entry (ctfc, section_name, info);
>>> }
>>> +
>>> + num_types_created += datasecs.length ();
>>> }
>>>
>>> /* Return true if the type ID is that of a type which will not be emitted (for
>>> @@ -461,29 +529,6 @@ btf_dtd_emit_preprocess_cb (ctf_container_ref ctfc, ctf_dtdef_ref dtd)
>>> if (!btf_emit_id_p (dtd->dtd_type))
>>> return;
>>>
>>> - uint32_t btf_kind
>>> - = get_btf_kind (CTF_V2_INFO_KIND (dtd->dtd_data.ctti_info));
>>> -
>>> - if (btf_kind == BTF_KIND_FUNC_PROTO)
>>> - {
>>> - /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
>>> - also a BTF_KIND_FUNC. But the CTF container only allocates one
>>> - type per function, which matches closely with BTF_KIND_FUNC_PROTO.
>>> - For each such function, also allocate a BTF_KIND_FUNC entry.
>>> - These will be output later. */
>>> - ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
>>> - func_dtd->dtd_data = dtd->dtd_data;
>>> - func_dtd->dtd_data.ctti_type = dtd->dtd_type;
>>> - func_dtd->linkage = dtd->linkage;
>>> -
>>> - vec_safe_push (funcs, func_dtd);
>>> - num_types_created++;
>>> -
>>> - /* Only the BTF_KIND_FUNC type actually references the name. The
>>> - BTF_KIND_FUNC_PROTO is always anonymous. */
>>> - dtd->dtd_data.ctti_name = 0;
>>> - }
>>> -
>>> ctfc->ctfc_num_vlen_bytes += btf_calc_num_vbytes (dtd);
>>> }
>>>
>>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>>> new file mode 100644
>>> index 00000000000..f4b298cf019
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>>> @@ -0,0 +1,28 @@
>>> +/* Test BTF generation of DATASEC records for extern functions.
>>> +
>>> + Only functions declared extern should have entries in DATASEC records. */
>>> +
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O0 -gbtf -dA" } */
>>> +
>>> +/* Expect one DATASEC with vlen=1 (.foo_sec) and one with vlen=2 (.bar_sec) */
>>> +/* { dg-final { scan-assembler-times "0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
>>> +/* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
>>> +
>>> +/* Function entries should have offset and size of 0 at compile time. */
>>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
>>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
>>> +
>>> +extern int foo (int a) __attribute__((section(".foo_sec")));
>>> +
>>> +
>>> +extern int bar (int b) __attribute__((section(".bar_sec")));
>>> +extern void chacha (void) __attribute__((section(".bar_sec")));
>>> +
>>> +__attribute__((section(".foo_sec")))
>>> +void baz (int *x)
>>> +{
>>> + chacha ();
>>> +
>>> + *x = foo (bar (*x));
>>> +}
>>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>>> new file mode 100644
>>> index 00000000000..48a946ab14b
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>>> @@ -0,0 +1,19 @@
>>> +/* Test BTF extern linkage for functions.
>>> +
>>> + We expect to see one BTF_KIND_FUNC type with global linkage (foo), and
>>> + one BTF_KIND_FUNC type with extern linkage (extfunc). */
>>> +
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O0 -gbtf -dA" } */
>>> +
>>> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=2" 1 } } */
>>> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=1" 1 } } */
>>> +
>>> +extern int extfunc(int a, int b);
>>> +
>>> +int foo (int x) {
>>> +
>>> + int y = extfunc (x, x+1);
>>> +
>>> + return y;
>>> +}
>>> diff --git a/include/btf.h b/include/btf.h
>>> index 9a757ce5bc9..f41ea94b75f 100644
>>> --- a/include/btf.h
>>> +++ b/include/btf.h
>>> @@ -186,12 +186,13 @@ struct btf_var
>>> };
>>>
>>> /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
>>> - which describe all BTF_KIND_VAR types contained in the section. */
>>> + which describe all BTF_KIND_VAR or extern BTF_KIND_FUNC types contained
>>> + in the section. */
>>> struct btf_var_secinfo
>>> {
>>> - uint32_t type; /* Type of variable. */
>>> - uint32_t offset; /* In-section offset of variable (in bytes). */
>>> - uint32_t size; /* Size (in bytes) of variable. */
>>> + uint32_t type; /* Type of BTF_KIND_VAR or BTF_KIND_FUNC item. */
>>> + uint32_t offset; /* In-section offset (in bytes) of item. */
>>> + uint32_t size; /* Size (in bytes) of item. */
>>> };
>>>
>>> /* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries,
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773]
2022-12-12 20:47 ` David Faust
@ 2022-12-13 6:15 ` Indu Bhagat
0 siblings, 0 replies; 13+ messages in thread
From: Indu Bhagat @ 2022-12-13 6:15 UTC (permalink / raw)
To: David Faust; +Cc: jose.marchesi, gcc-patches
On 12/12/22 12:47, David Faust wrote:
>
>
> On 12/8/22 22:55, Indu Bhagat wrote:
>> Hi David,
>>
>> On 12/7/22 12:57, David Faust wrote:
>>> Add support for the 'extern' linkage value for BTF_KIND_VAR records,
>>> which is used for variables declared as extern in the source file.
>>>
>>> PR target/106773
>>>
>>> gcc/
>>>
>>> * btfout.cc (BTF_LINKAGE_STATIC): New define.
>>> (BTF_LINKAGE_GLOBAL): Likewise.
>>> (BTF_LINKAGE_EXTERN): Likewise.
>>> (btf_collect_datasec): Mark extern variables as such.
>>> (btf_asm_varent): Accomodate 'extern' linkage.
>>>
>>> gcc/testsuite/
>>>
>>> * gcc.dg/debug/btf/btf-variables-4.c: New test.
>>>
>>> include/
>>>
>>> * btf.h (struct btf_var): Update comment to note 'extern' linkage.
>>> ---
>>> gcc/btfout.cc | 9 ++++++-
>>> .../gcc.dg/debug/btf/btf-variables-4.c | 24 +++++++++++++++++++
>>> include/btf.h | 2 +-
>>> 3 files changed, 33 insertions(+), 2 deletions(-)
>>> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>>>
>>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>>> index aef9fd70a28..a1c6266a7db 100644
>>> --- a/gcc/btfout.cc
>>> +++ b/gcc/btfout.cc
>>> @@ -66,6 +66,10 @@ static char btf_info_section_label[MAX_BTF_LABEL_BYTES];
>>>
>>> #define BTF_INVALID_TYPEID 0xFFFFFFFF
>>>
>>> +#define BTF_LINKAGE_STATIC 0
>>> +#define BTF_LINKAGE_GLOBAL 1
>>> +#define BTF_LINKAGE_EXTERN 2
>>> +
>>
>> I was about to suggest to rename these to use the same name as used in
>> the kernel btf.h. What is used there is:
>> BTF_VAR_STATIC = 0,
>> BTF_VAR_GLOBAL_ALLOCATED = 1,
>> BTF_VAR_GLOBAL_EXTERN = 2,
>>
>> But after looking at the Patch 3/3, I see you reuse these definitions
>> for functions as well. I just find the names confusing on the first look
>> - "BTF_LINKAGE_STATIC".
>>
>> Naming aside, what do you think about adding the defines to
>> include/btf.h instead ?
>
> Actually, I forgot these are defined (separately for both VARs and FUNCs)
> in the kernel uapi/linux/btf.h. It would probably be best to mirror that
> approach and use a separate enum for each, in include/btf.h. WDYT?
>
Yes, mirroring in include/btf.h sounds good.
>>
>>> /* Mapping of CTF variables to the IDs they will be assigned when they are
>>> converted to BTF_KIND_VAR type records. Strictly accounts for the index
>>> from the start of the variable type entries, does not include the number
>>> @@ -314,6 +318,9 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>> continue;
>>>
>>> const char *section_name = node->get_section ();
>>> + /* Mark extern variables. */
>>> + if (DECL_EXTERNAL (node->decl))
>>> + dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>>>
>>
>> This made me think about the following case.
>>
>> extern const char a[];
>> const char a[] = "foo";
>>
>> What is the expected BTF for this? Since BTF can differentiate between
>> the non-defining extern variable declaration, I expected to see two
>> variables with different "linkage". At this time I see, two variables
>> with global linkage but different types:
>>
>> .long 0xe000000 # btv_info
>> .long 0x4 # btv_type
>> .long 0x1 # btv_linkage
>> .long 0x1f # btv_name
>> .long 0xe000000 # btv_info
>> .long 0x7 # btv_type
>> .long 0x1 # btv_linkage
>> .long 0x60 # btt_name
>>
>
> The BTF documentation in the kernel does not clarify this case.
> Going off the implementation in clang as a reference, it looks like
> only one VAR record is expected, with 'global' linkage:
>
> $ cat extdef.c
> extern const char a[];
> const char a[] = "foo";
>
> $ clang -target bpf -c -g extdef.c -o extdef.o
>
> $ /usr/sbin/bpftool btf dump file extdef.o
> [1] CONST '(anon)' type_id=2
> [2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED
> [3] ARRAY '(anon)' type_id=1 index_type_id=4 nr_elems=4
> [4] INT '__ARRAY_SIZE_TYPE__' size=4 bits_offset=0 nr_bits=32 encoding=(none)
> [5] VAR 'a' type_id=3, linkage=global
> [6] DATASEC '.rodata' size=0 vlen=1
> type_id=5 offset=0 size=4 (VAR 'a')
>
> In GCC we have two records since we have two DIEs for "a" in the
> DWARF. One has type "const char[4]" and the other has type
> "const char[]", so the BTF records point to two different types
> as well.
>
> I guess we should find a way in BTF to identify this and
> emit only the defining definition as clang does.
>
>
CTF had a similar issue earlier. See PR105089.
https://gcc.gnu.org/PR105089
>>> if (section_name == NULL)
>>> {
>>> @@ -676,7 +683,7 @@ btf_asm_varent (ctf_dvdef_ref var)
>>> dw2_asm_output_data (4, var->dvd_name_offset, "btv_name");
>>> dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_VAR, 0, 0), "btv_info");
>>> dw2_asm_output_data (4, get_btf_id (var->dvd_type), "btv_type");
>>> - dw2_asm_output_data (4, (var->dvd_visibility ? 1 : 0), "btv_linkage");
>>> + dw2_asm_output_data (4, var->dvd_visibility, "btv_linkage");
>>> }
>>>
>>> /* Asm'out a member description following a BTF_KIND_STRUCT or
>>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>>> new file mode 100644
>>> index 00000000000..d77600bae1c
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>>> @@ -0,0 +1,24 @@
>>> +/* Test BTF generation for extern variables. */
>>> +
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O0 -gbtf -dA" } */
>>> +
>>> +/* Expect 4 variables. */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 4 } } */
>>> +
>>> +/* 2 extern, 1 global, 1 static. */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0x1\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 2 } } */
>>> +
>>> +extern int a;
>>> +extern const int b;
>>> +int c;
>>> +static const int d = 5;
>>> +
>>> +int foo (int x)
>>> +{
>>> + c = a + b + x;
>>> +
>>> + return c + d;
>>> +}
>>> diff --git a/include/btf.h b/include/btf.h
>>> index eba67f9d599..9a757ce5bc9 100644
>>> --- a/include/btf.h
>>> +++ b/include/btf.h
>>> @@ -182,7 +182,7 @@ struct btf_param
>>> information about the variable. */
>>> struct btf_var
>>> {
>>> - uint32_t linkage; /* Currently only 0=static or 1=global. */
>>> + uint32_t linkage; /* 0=static, 1=global, 2=extern. */
>>> };
>>>
>>> /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-12-13 6:16 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-07 20:57 [PATCH 0/3] btf: fix BTF for extern items [PR106773] David Faust
2022-12-07 20:57 ` [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773] David Faust
2022-12-09 6:55 ` Indu Bhagat
2022-12-12 20:47 ` David Faust
2022-12-13 6:15 ` Indu Bhagat
2022-12-07 20:57 ` [PATCH 2/3] btf: fix 'extern const void' " David Faust
2022-12-09 7:34 ` Indu Bhagat
2022-12-12 20:59 ` David Faust
2022-12-13 6:11 ` Indu Bhagat
2022-12-07 20:57 ` [PATCH 3/3] btf: correct generation for extern funcs [PR106773] David Faust
2022-12-09 7:36 ` Indu Bhagat
2022-12-12 20:31 ` David Faust
2022-12-13 6:12 ` Indu Bhagat
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).