Message ID | 20231204234715.9773-1-david.faust@oracle.com |
---|---|
State | New |
Headers | show |
Series | btf: avoid wrong DATASEC entries for extern vars [PR112849] | expand |
On 12/4/23 15:47, David Faust wrote: > The process of creating BTF_KIND_DATASEC records involves iterating > through variable declarations, determining which section they will be > placed in, and creating an entry in the appropriate DATASEC record > accordingly. > > For variables without e.g. an explicit __attribute__((section)), we use > categorize_decl_for_section () to identify the appropriate named section > and corresponding BTF_KIND_DATASEC record. > > This was incorrectly being done for 'extern' variable declarations as > well as non-extern ones, which meant that extern variable declarations > could result in BTF_KIND_DATASEC entries claiming the variable is > allocated in some section such as '.bss' without any knowledge whether > that is actually true. That resulted in errors building the Linux kernel > BPF selftests. > > This patch corrects btf_collect_datasec () to avoid assuming a section > for extern variables, and only emit BTF_KIND_DATASEC entries for them if > they have a known section. > > Bootstrapped + tested on x86_64-linux-gnu. > Tested on x86_64-linux-gnu host for bpf-unknown-none. > One comment below. LGTM, otherwise. Thanks > gcc/ > PR debug/112849 > * btfout.cc (btf_collect_datasec): Avoid incorrectly creating an > entry in a BTF_KIND_DATASEC record for extern variable decls without > a known section. > > gcc/testsuite/ > PR debug/112849 > * gcc.dg/debug/btf/btf-datasec-3.c: New test. > --- > gcc/btfout.cc | 10 ++++++- > .../gcc.dg/debug/btf/btf-datasec-3.c | 27 +++++++++++++++++++ > 2 files changed, 36 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c > > diff --git a/gcc/btfout.cc b/gcc/btfout.cc > index a5e0d640e19..db4f1084f85 100644 > --- a/gcc/btfout.cc > +++ b/gcc/btfout.cc > @@ -486,7 +486,15 @@ btf_collect_datasec (ctf_container_ref ctfc) > > /* Mark extern variables. */ > if (DECL_EXTERNAL (node->decl)) > - dvd->dvd_visibility = BTF_VAR_GLOBAL_EXTERN; > + { > + dvd->dvd_visibility = BTF_VAR_GLOBAL_EXTERN; > + > + /* PR112849: avoid assuming a section for extern decls without > + an explicit section, which would result in incorrectly > + emitting a BTF_KIND_DATASEC entry for them. */ > + if (node->get_section () == NULL) > + continue; > + } > > const char *section_name = get_section_name (node); > if (section_name == NULL) > diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c > new file mode 100644 > index 00000000000..3c1c7a28c2a > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c > @@ -0,0 +1,27 @@ > +/* PR debug/112849 > + Test that we do not incorrectly create BTF_KIND_DATASEC entries for > + extern decls with no known section. */ > + > +/* { dg-do compile } */ > +/* { dg-options "-O0 -gbtf -dA" } */ > + > +extern int VERSION __attribute__((section (".version"))); > + > +extern int test_bss1; > +extern int test_data1; > + > +int test_bss2; > +int test_data2 = 2; > + > +int > +foo (void) > +{ > + test_bss2 = VERSION; > + return test_bss1 + test_data1 + test_data2; > +} > + > +/* There should only be a DATASEC entries for VERSION out of the extern decls. */ The statement is unclear as is. Perhaps you wanted to say "There should only be 3 DATASEC entries; including one for VERSION even though it is extern decl" ? > +/* { dg-final { scan-assembler-times "bts_type" 3 } } */ > +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR 'test_data2'\\)" 1 } } */ > +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR 'test_bss2'\\)" 1 } } */ > +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR 'VERSION'\\)" 1 } } */
On 12/5/23 13:28, Indu Bhagat wrote: > On 12/4/23 15:47, David Faust wrote: >> The process of creating BTF_KIND_DATASEC records involves iterating >> through variable declarations, determining which section they will be >> placed in, and creating an entry in the appropriate DATASEC record >> accordingly. >> >> For variables without e.g. an explicit __attribute__((section)), we use >> categorize_decl_for_section () to identify the appropriate named section >> and corresponding BTF_KIND_DATASEC record. >> >> This was incorrectly being done for 'extern' variable declarations as >> well as non-extern ones, which meant that extern variable declarations >> could result in BTF_KIND_DATASEC entries claiming the variable is >> allocated in some section such as '.bss' without any knowledge whether >> that is actually true. That resulted in errors building the Linux kernel >> BPF selftests. >> >> This patch corrects btf_collect_datasec () to avoid assuming a section >> for extern variables, and only emit BTF_KIND_DATASEC entries for them if >> they have a known section. >> >> Bootstrapped + tested on x86_64-linux-gnu. >> Tested on x86_64-linux-gnu host for bpf-unknown-none. >> > > One comment below. > > LGTM, otherwise. > Thanks > >> gcc/ >> PR debug/112849 >> * btfout.cc (btf_collect_datasec): Avoid incorrectly creating an >> entry in a BTF_KIND_DATASEC record for extern variable decls without >> a known section. >> >> gcc/testsuite/ >> PR debug/112849 >> * gcc.dg/debug/btf/btf-datasec-3.c: New test. >> --- >> gcc/btfout.cc | 10 ++++++- >> .../gcc.dg/debug/btf/btf-datasec-3.c | 27 +++++++++++++++++++ >> 2 files changed, 36 insertions(+), 1 deletion(-) >> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c >> >> diff --git a/gcc/btfout.cc b/gcc/btfout.cc >> index a5e0d640e19..db4f1084f85 100644 >> --- a/gcc/btfout.cc >> +++ b/gcc/btfout.cc >> @@ -486,7 +486,15 @@ btf_collect_datasec (ctf_container_ref ctfc) >> >> /* Mark extern variables. */ >> if (DECL_EXTERNAL (node->decl)) >> - dvd->dvd_visibility = BTF_VAR_GLOBAL_EXTERN; >> + { >> + dvd->dvd_visibility = BTF_VAR_GLOBAL_EXTERN; >> + >> + /* PR112849: avoid assuming a section for extern decls without >> + an explicit section, which would result in incorrectly >> + emitting a BTF_KIND_DATASEC entry for them. */ >> + if (node->get_section () == NULL) >> + continue; >> + } >> >> const char *section_name = get_section_name (node); >> if (section_name == NULL) >> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c >> new file mode 100644 >> index 00000000000..3c1c7a28c2a >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c >> @@ -0,0 +1,27 @@ >> +/* PR debug/112849 >> + Test that we do not incorrectly create BTF_KIND_DATASEC entries for >> + extern decls with no known section. */ >> + >> +/* { dg-do compile } */ >> +/* { dg-options "-O0 -gbtf -dA" } */ >> + >> +extern int VERSION __attribute__((section (".version"))); >> + >> +extern int test_bss1; >> +extern int test_data1; >> + >> +int test_bss2; >> +int test_data2 = 2; >> + >> +int >> +foo (void) >> +{ >> + test_bss2 = VERSION; >> + return test_bss1 + test_data1 + test_data2; >> +} >> + >> +/* There should only be a DATASEC entries for VERSION out of the extern decls. */ > > The statement is unclear as is. Perhaps you wanted to say "There should > only be 3 DATASEC entries; including one for VERSION even though it is > extern decl" ? Thanks. I reworded parts of that once or twice ended up with a garbled mess. Changed to: /* There should be 3 DATASEC entries total. Of the extern decls, only VERSION has a known section; entries are not created for the other two. */ and pushed. > >> +/* { dg-final { scan-assembler-times "bts_type" 3 } } */ >> +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR 'test_data2'\\)" 1 } } */ >> +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR 'test_bss2'\\)" 1 } } */ >> +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR 'VERSION'\\)" 1 } } */ >
diff --git a/gcc/btfout.cc b/gcc/btfout.cc index a5e0d640e19..db4f1084f85 100644 --- a/gcc/btfout.cc +++ b/gcc/btfout.cc @@ -486,7 +486,15 @@ btf_collect_datasec (ctf_container_ref ctfc) /* Mark extern variables. */ if (DECL_EXTERNAL (node->decl)) - dvd->dvd_visibility = BTF_VAR_GLOBAL_EXTERN; + { + dvd->dvd_visibility = BTF_VAR_GLOBAL_EXTERN; + + /* PR112849: avoid assuming a section for extern decls without + an explicit section, which would result in incorrectly + emitting a BTF_KIND_DATASEC entry for them. */ + if (node->get_section () == NULL) + continue; + } const char *section_name = get_section_name (node); if (section_name == NULL) diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c new file mode 100644 index 00000000000..3c1c7a28c2a --- /dev/null +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c @@ -0,0 +1,27 @@ +/* PR debug/112849 + Test that we do not incorrectly create BTF_KIND_DATASEC entries for + extern decls with no known section. */ + +/* { dg-do compile } */ +/* { dg-options "-O0 -gbtf -dA" } */ + +extern int VERSION __attribute__((section (".version"))); + +extern int test_bss1; +extern int test_data1; + +int test_bss2; +int test_data2 = 2; + +int +foo (void) +{ + test_bss2 = VERSION; + return test_bss1 + test_data1 + test_data2; +} + +/* There should only be a DATASEC entries for VERSION out of the extern decls. */ +/* { dg-final { scan-assembler-times "bts_type" 3 } } */ +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR 'test_data2'\\)" 1 } } */ +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR 'test_bss2'\\)" 1 } } */ +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR 'VERSION'\\)" 1 } } */