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,
>
