Message ID | 20201208125128.775313-2-hjl.tools@gmail.com |
---|---|
State | New |
Headers | show |
Series | V3 [PATCH 1/2] Switch to a new section if the SECTION_RETAIN bit doesn't match | expand |
This patch fixes the section type conflict that I have been seeing. I have tested this patch using : Build: x86_64 Host: x86_64 Target: aarch64-none-linux-gnu On 08/12/2020, 12:52, "Gcc-patches on behalf of H.J. Lu via Gcc-patches" <gcc-patches-bounces@gcc.gnu.org on behalf of gcc-patches@gcc.gnu.org> wrote: When definitions marked with used attribute and unmarked definitions are placed in the section with the same name, switch to a new section if the SECTION_RETAIN bit doesn't match. gcc/ PR target/98146 * output.h (switch_to_section): Add a tree argument, default to nullptr. * varasm.c (get_section): If the SECTION_RETAIN bit doesn't match, return and switch to a new section later. (assemble_start_function): Pass decl to switch_to_section. (assemble_variable): Likewise. (switch_to_section): If the SECTION_RETAIN bit doesn't match, switch to a new section. gcc/testsuite/ PR target/98146 * c-c++-common/attr-used-5.c: New test. * c-c++-common/attr-used-6.c: Likewise. * c-c++-common/attr-used-7.c: Likewise. * c-c++-common/attr-used-8.c: Likewise. * c-c++-common/attr-used-9.c: Likewise. --- gcc/output.h | 2 +- gcc/testsuite/c-c++-common/attr-used-5.c | 26 ++++++++++++++++++++ gcc/testsuite/c-c++-common/attr-used-6.c | 26 ++++++++++++++++++++ gcc/testsuite/c-c++-common/attr-used-7.c | 8 +++++++ gcc/testsuite/c-c++-common/attr-used-8.c | 8 +++++++ gcc/testsuite/c-c++-common/attr-used-9.c | 28 ++++++++++++++++++++++ gcc/varasm.c | 30 ++++++++++++++++++++---- 7 files changed, 123 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/attr-used-5.c create mode 100644 gcc/testsuite/c-c++-common/attr-used-6.c create mode 100644 gcc/testsuite/c-c++-common/attr-used-7.c create mode 100644 gcc/testsuite/c-c++-common/attr-used-8.c create mode 100644 gcc/testsuite/c-c++-common/attr-used-9.c diff --git a/gcc/output.h b/gcc/output.h index fa8ace1f394..1f9af46da1d 100644 --- a/gcc/output.h +++ b/gcc/output.h @@ -548,7 +548,7 @@ extern void switch_to_other_text_partition (void); extern section *get_cdtor_priority_section (int, bool); extern bool unlikely_text_section_p (section *); -extern void switch_to_section (section *); +extern void switch_to_section (section *, tree = nullptr); extern void output_section_asm_op (const void *); extern void record_tm_clone_pair (tree, tree); diff --git a/gcc/testsuite/c-c++-common/attr-used-5.c b/gcc/testsuite/c-c++-common/attr-used-5.c new file mode 100644 index 00000000000..9fc0d3834e9 --- /dev/null +++ b/gcc/testsuite/c-c++-common/attr-used-5.c @@ -0,0 +1,26 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -O2" } */ + +struct dtv_slotinfo_list +{ + struct dtv_slotinfo_list *next; +}; + +extern struct dtv_slotinfo_list *list; + +static int __attribute__ ((section ("__libc_freeres_fn"))) +free_slotinfo (struct dtv_slotinfo_list **elemp) +{ + if (!free_slotinfo (&(*elemp)->next)) + return 0; + return 1; +} + +__attribute__ ((used, section ("__libc_freeres_fn"))) +static void free_mem (void) +{ + free_slotinfo (&list); +} + +/* { dg-final { scan-assembler "__libc_freeres_fn,\"ax\"" { target R_flag_in_section } } } */ +/* { dg-final { scan-assembler "__libc_freeres_fn,\"axR\"" { target R_flag_in_section } } } */ diff --git a/gcc/testsuite/c-c++-common/attr-used-6.c b/gcc/testsuite/c-c++-common/attr-used-6.c new file mode 100644 index 00000000000..0cb82ade5a9 --- /dev/null +++ b/gcc/testsuite/c-c++-common/attr-used-6.c @@ -0,0 +1,26 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -O2" } */ + +struct dtv_slotinfo_list +{ + struct dtv_slotinfo_list *next; +}; + +extern struct dtv_slotinfo_list *list; + +static int __attribute__ ((used, section ("__libc_freeres_fn"))) +free_slotinfo (struct dtv_slotinfo_list **elemp) +{ + if (!free_slotinfo (&(*elemp)->next)) + return 0; + return 1; +} + +__attribute__ ((section ("__libc_freeres_fn"))) +void free_mem (void) +{ + free_slotinfo (&list); +} + +/* { dg-final { scan-assembler "__libc_freeres_fn,\"ax\"" { target R_flag_in_section } } } */ +/* { dg-final { scan-assembler "__libc_freeres_fn,\"axR\"" { target R_flag_in_section } } } */ diff --git a/gcc/testsuite/c-c++-common/attr-used-7.c b/gcc/testsuite/c-c++-common/attr-used-7.c new file mode 100644 index 00000000000..fba2706ffc1 --- /dev/null +++ b/gcc/testsuite/c-c++-common/attr-used-7.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -O2" } */ + +int __attribute__((used,section(".data.foo"))) foo2 = 2; +int __attribute__((section(".data.foo"))) foo1 = 1; + +/* { dg-final { scan-assembler ".data.foo,\"aw\"" { target R_flag_in_section } } } */ +/* { dg-final { scan-assembler ".data.foo,\"awR\"" { target R_flag_in_section } } } */ diff --git a/gcc/testsuite/c-c++-common/attr-used-8.c b/gcc/testsuite/c-c++-common/attr-used-8.c new file mode 100644 index 00000000000..4da4aabe573 --- /dev/null +++ b/gcc/testsuite/c-c++-common/attr-used-8.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -O2" } */ + +int __attribute__((section(".data.foo"))) foo1 = 1; +int __attribute__((used,section(".data.foo"))) foo2 = 2; + +/* { dg-final { scan-assembler ".data.foo,\"aw\"" { target R_flag_in_section } } } */ +/* { dg-final { scan-assembler ".data.foo,\"awR\"" { target R_flag_in_section } } } */ diff --git a/gcc/testsuite/c-c++-common/attr-used-9.c b/gcc/testsuite/c-c++-common/attr-used-9.c new file mode 100644 index 00000000000..cf3bde67622 --- /dev/null +++ b/gcc/testsuite/c-c++-common/attr-used-9.c @@ -0,0 +1,28 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -O2" } */ + +struct dtv_slotinfo_list +{ + struct dtv_slotinfo_list *next; +}; + +extern struct dtv_slotinfo_list *list; + +static int __attribute__ ((used, section ("__libc_freeres_fn"))) +free_slotinfo (struct dtv_slotinfo_list **elemp) +{ + if (!free_slotinfo (&(*elemp)->next)) + return 0; + return 1; +} + +__attribute__ ((section ("__libc_freeres_fn"))) +static void free_mem (void) +/* { dg-warning "defined but not used" "" { target *-*-* } .-1 } */ +{ + free_slotinfo (&list); +} + +/* { dg-final { scan-assembler-not "__libc_freeres_fn\n" } } */ +/* { dg-final { scan-assembler-not "__libc_freeres_fn,\"ax\"" { target R_flag_in_section } } } */ +/* { dg-final { scan-assembler "__libc_freeres_fn,\"axR\"" { target R_flag_in_section } } } */ diff --git a/gcc/varasm.c b/gcc/varasm.c index 0fac3688828..c5ea29c4e4c 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -342,6 +342,11 @@ get_section (const char *name, unsigned int flags, tree decl, sect->common.flags |= (SECTION_WRITE | SECTION_RELRO); return sect; } + /* If the SECTION_RETAIN bit doesn't match, return and switch + to a new section later. */ + if ((sect->common.flags & SECTION_RETAIN) + != (flags & SECTION_RETAIN)) + return sect; /* Sanity check user variables for flag changes. */ if (sect->named.decl != NULL && DECL_P (sect->named.decl) @@ -1852,7 +1857,7 @@ assemble_start_function (tree decl, const char *fnname) /* Switch to the correct text section for the start of the function. */ - switch_to_section (function_section (decl)); + switch_to_section (function_section (decl), decl); if (crtl->has_bb_partition && !hot_label_written) ASM_OUTPUT_LABEL (asm_out_file, crtl->subsections.hot_section_label); @@ -2348,7 +2353,7 @@ assemble_variable (tree decl, int top_level ATTRIBUTE_UNUSED, && (strcmp (sect->named.name, ".vtable_map_vars") == 0)) handle_vtv_comdat_section (sect, decl); else - switch_to_section (sect); + switch_to_section (sect, decl); if (align > BITS_PER_UNIT) ASM_OUTPUT_ALIGN (asm_out_file, floor_log2 (align / BITS_PER_UNIT)); assemble_variable_contents (decl, name, dont_output_data, @@ -7715,10 +7720,27 @@ output_section_asm_op (const void *directive) the current section is NEW_SECTION. */ void -switch_to_section (section *new_section) +switch_to_section (section *new_section, tree decl) { if (in_section == new_section) - return; + { + if (HAVE_GAS_SHF_GNU_RETAIN + && (new_section->common.flags & SECTION_NAMED) + && decl != nullptr + && (!!DECL_PRESERVE_P (decl) + != !!(new_section->common.flags & SECTION_RETAIN))) + { + /* If the SECTION_RETAIN bit doesn't match, switch to a new + section. */ + if (DECL_PRESERVE_P (decl)) + new_section->common.flags |= SECTION_RETAIN; + else + new_section->common.flags &= ~(SECTION_RETAIN + | SECTION_DECLARED); + } + else + return; + } if (new_section->common.flags & SECTION_FORGET) in_section = NULL; -- 2.28.0
On 12/8/20 5:51 AM, H.J. Lu wrote: > When definitions marked with used attribute and unmarked definitions are > placed in the section with the same name, switch to a new section if the > SECTION_RETAIN bit doesn't match. > > gcc/ > > PR target/98146 > * output.h (switch_to_section): Add a tree argument, default to > nullptr. > * varasm.c (get_section): If the SECTION_RETAIN bit doesn't match, > return and switch to a new section later. > (assemble_start_function): Pass decl to switch_to_section. > (assemble_variable): Likewise. > (switch_to_section): If the SECTION_RETAIN bit doesn't match, > switch to a new section. > > gcc/testsuite/ > > PR target/98146 > * c-c++-common/attr-used-5.c: New test. > * c-c++-common/attr-used-6.c: Likewise. > * c-c++-common/attr-used-7.c: Likewise. > * c-c++-common/attr-used-8.c: Likewise. > * c-c++-common/attr-used-9.c: Likewise. > --- > gcc/output.h | 2 +- > gcc/testsuite/c-c++-common/attr-used-5.c | 26 ++++++++++++++++++++ > gcc/testsuite/c-c++-common/attr-used-6.c | 26 ++++++++++++++++++++ > gcc/testsuite/c-c++-common/attr-used-7.c | 8 +++++++ > gcc/testsuite/c-c++-common/attr-used-8.c | 8 +++++++ > gcc/testsuite/c-c++-common/attr-used-9.c | 28 ++++++++++++++++++++++ > gcc/varasm.c | 30 ++++++++++++++++++++---- > 7 files changed, 123 insertions(+), 5 deletions(-) > create mode 100644 gcc/testsuite/c-c++-common/attr-used-5.c > create mode 100644 gcc/testsuite/c-c++-common/attr-used-6.c > create mode 100644 gcc/testsuite/c-c++-common/attr-used-7.c > create mode 100644 gcc/testsuite/c-c++-common/attr-used-8.c > create mode 100644 gcc/testsuite/c-c++-common/attr-used-9.c > > > diff --git a/gcc/varasm.c b/gcc/varasm.c > index 0fac3688828..c5ea29c4e4c 100644 > --- a/gcc/varasm.c > +++ b/gcc/varasm.c > @@ -342,6 +342,11 @@ get_section (const char *name, unsigned int flags, tree decl, > sect->common.flags |= (SECTION_WRITE | SECTION_RELRO); > return sect; > } > + /* If the SECTION_RETAIN bit doesn't match, return and switch > + to a new section later. */ > + if ((sect->common.flags & SECTION_RETAIN) > + != (flags & SECTION_RETAIN)) > + return sect; At the least you need to fix the function comment as it would no longer match the behavior in this case. It also seems to me that having SECTION_RETAIN be a special case like this just seems wrong. In my mind there isn't anything fundamentally different with SECTION_RETAIN vs other symbols that it deserves to be special cased like this. Clearly we need to fix something as these bits are causing significant fallout, but I'm just not sure what the right fix ought to be. jeff
On Mon, Dec 14, 2020 at 5:06 PM Jeff Law <law@redhat.com> wrote: > > > > On 12/8/20 5:51 AM, H.J. Lu wrote: > > When definitions marked with used attribute and unmarked definitions are > > placed in the section with the same name, switch to a new section if the > > SECTION_RETAIN bit doesn't match. > > > > gcc/ > > > > PR target/98146 > > * output.h (switch_to_section): Add a tree argument, default to > > nullptr. > > * varasm.c (get_section): If the SECTION_RETAIN bit doesn't match, > > return and switch to a new section later. > > (assemble_start_function): Pass decl to switch_to_section. > > (assemble_variable): Likewise. > > (switch_to_section): If the SECTION_RETAIN bit doesn't match, > > switch to a new section. > > > > gcc/testsuite/ > > > > PR target/98146 > > * c-c++-common/attr-used-5.c: New test. > > * c-c++-common/attr-used-6.c: Likewise. > > * c-c++-common/attr-used-7.c: Likewise. > > * c-c++-common/attr-used-8.c: Likewise. > > * c-c++-common/attr-used-9.c: Likewise. > > --- > > gcc/output.h | 2 +- > > gcc/testsuite/c-c++-common/attr-used-5.c | 26 ++++++++++++++++++++ > > gcc/testsuite/c-c++-common/attr-used-6.c | 26 ++++++++++++++++++++ > > gcc/testsuite/c-c++-common/attr-used-7.c | 8 +++++++ > > gcc/testsuite/c-c++-common/attr-used-8.c | 8 +++++++ > > gcc/testsuite/c-c++-common/attr-used-9.c | 28 ++++++++++++++++++++++ > > gcc/varasm.c | 30 ++++++++++++++++++++---- > > 7 files changed, 123 insertions(+), 5 deletions(-) > > create mode 100644 gcc/testsuite/c-c++-common/attr-used-5.c > > create mode 100644 gcc/testsuite/c-c++-common/attr-used-6.c > > create mode 100644 gcc/testsuite/c-c++-common/attr-used-7.c > > create mode 100644 gcc/testsuite/c-c++-common/attr-used-8.c > > create mode 100644 gcc/testsuite/c-c++-common/attr-used-9.c > > > > > > diff --git a/gcc/varasm.c b/gcc/varasm.c > > index 0fac3688828..c5ea29c4e4c 100644 > > --- a/gcc/varasm.c > > +++ b/gcc/varasm.c > > @@ -342,6 +342,11 @@ get_section (const char *name, unsigned int flags, tree decl, > > sect->common.flags |= (SECTION_WRITE | SECTION_RELRO); > > return sect; > > } > > + /* If the SECTION_RETAIN bit doesn't match, return and switch > > + to a new section later. */ > > + if ((sect->common.flags & SECTION_RETAIN) > > + != (flags & SECTION_RETAIN)) > > + return sect; > At the least you need to fix the function comment as it would no longer > match the behavior in this case. It also seems to me that having > SECTION_RETAIN be a special case like this just seems wrong. In my > mind there isn't anything fundamentally different with SECTION_RETAIN vs > other symbols that it deserves to be special cased like this. Any symbols with SHF_XXX can be special, for example SHF_TLS. > Clearly we need to fix something as these bits are causing significant > fallout, but I'm just not sure what the right fix ought to be. > > jeff >
diff --git a/gcc/output.h b/gcc/output.h index fa8ace1f394..1f9af46da1d 100644 --- a/gcc/output.h +++ b/gcc/output.h @@ -548,7 +548,7 @@ extern void switch_to_other_text_partition (void); extern section *get_cdtor_priority_section (int, bool); extern bool unlikely_text_section_p (section *); -extern void switch_to_section (section *); +extern void switch_to_section (section *, tree = nullptr); extern void output_section_asm_op (const void *); extern void record_tm_clone_pair (tree, tree); diff --git a/gcc/testsuite/c-c++-common/attr-used-5.c b/gcc/testsuite/c-c++-common/attr-used-5.c new file mode 100644 index 00000000000..9fc0d3834e9 --- /dev/null +++ b/gcc/testsuite/c-c++-common/attr-used-5.c @@ -0,0 +1,26 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -O2" } */ + +struct dtv_slotinfo_list +{ + struct dtv_slotinfo_list *next; +}; + +extern struct dtv_slotinfo_list *list; + +static int __attribute__ ((section ("__libc_freeres_fn"))) +free_slotinfo (struct dtv_slotinfo_list **elemp) +{ + if (!free_slotinfo (&(*elemp)->next)) + return 0; + return 1; +} + +__attribute__ ((used, section ("__libc_freeres_fn"))) +static void free_mem (void) +{ + free_slotinfo (&list); +} + +/* { dg-final { scan-assembler "__libc_freeres_fn,\"ax\"" { target R_flag_in_section } } } */ +/* { dg-final { scan-assembler "__libc_freeres_fn,\"axR\"" { target R_flag_in_section } } } */ diff --git a/gcc/testsuite/c-c++-common/attr-used-6.c b/gcc/testsuite/c-c++-common/attr-used-6.c new file mode 100644 index 00000000000..0cb82ade5a9 --- /dev/null +++ b/gcc/testsuite/c-c++-common/attr-used-6.c @@ -0,0 +1,26 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -O2" } */ + +struct dtv_slotinfo_list +{ + struct dtv_slotinfo_list *next; +}; + +extern struct dtv_slotinfo_list *list; + +static int __attribute__ ((used, section ("__libc_freeres_fn"))) +free_slotinfo (struct dtv_slotinfo_list **elemp) +{ + if (!free_slotinfo (&(*elemp)->next)) + return 0; + return 1; +} + +__attribute__ ((section ("__libc_freeres_fn"))) +void free_mem (void) +{ + free_slotinfo (&list); +} + +/* { dg-final { scan-assembler "__libc_freeres_fn,\"ax\"" { target R_flag_in_section } } } */ +/* { dg-final { scan-assembler "__libc_freeres_fn,\"axR\"" { target R_flag_in_section } } } */ diff --git a/gcc/testsuite/c-c++-common/attr-used-7.c b/gcc/testsuite/c-c++-common/attr-used-7.c new file mode 100644 index 00000000000..fba2706ffc1 --- /dev/null +++ b/gcc/testsuite/c-c++-common/attr-used-7.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -O2" } */ + +int __attribute__((used,section(".data.foo"))) foo2 = 2; +int __attribute__((section(".data.foo"))) foo1 = 1; + +/* { dg-final { scan-assembler ".data.foo,\"aw\"" { target R_flag_in_section } } } */ +/* { dg-final { scan-assembler ".data.foo,\"awR\"" { target R_flag_in_section } } } */ diff --git a/gcc/testsuite/c-c++-common/attr-used-8.c b/gcc/testsuite/c-c++-common/attr-used-8.c new file mode 100644 index 00000000000..4da4aabe573 --- /dev/null +++ b/gcc/testsuite/c-c++-common/attr-used-8.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -O2" } */ + +int __attribute__((section(".data.foo"))) foo1 = 1; +int __attribute__((used,section(".data.foo"))) foo2 = 2; + +/* { dg-final { scan-assembler ".data.foo,\"aw\"" { target R_flag_in_section } } } */ +/* { dg-final { scan-assembler ".data.foo,\"awR\"" { target R_flag_in_section } } } */ diff --git a/gcc/testsuite/c-c++-common/attr-used-9.c b/gcc/testsuite/c-c++-common/attr-used-9.c new file mode 100644 index 00000000000..cf3bde67622 --- /dev/null +++ b/gcc/testsuite/c-c++-common/attr-used-9.c @@ -0,0 +1,28 @@ +/* { dg-do compile } */ +/* { dg-options "-Wall -O2" } */ + +struct dtv_slotinfo_list +{ + struct dtv_slotinfo_list *next; +}; + +extern struct dtv_slotinfo_list *list; + +static int __attribute__ ((used, section ("__libc_freeres_fn"))) +free_slotinfo (struct dtv_slotinfo_list **elemp) +{ + if (!free_slotinfo (&(*elemp)->next)) + return 0; + return 1; +} + +__attribute__ ((section ("__libc_freeres_fn"))) +static void free_mem (void) +/* { dg-warning "defined but not used" "" { target *-*-* } .-1 } */ +{ + free_slotinfo (&list); +} + +/* { dg-final { scan-assembler-not "__libc_freeres_fn\n" } } */ +/* { dg-final { scan-assembler-not "__libc_freeres_fn,\"ax\"" { target R_flag_in_section } } } */ +/* { dg-final { scan-assembler "__libc_freeres_fn,\"axR\"" { target R_flag_in_section } } } */ diff --git a/gcc/varasm.c b/gcc/varasm.c index 0fac3688828..c5ea29c4e4c 100644 --- a/gcc/varasm.c +++ b/gcc/varasm.c @@ -342,6 +342,11 @@ get_section (const char *name, unsigned int flags, tree decl, sect->common.flags |= (SECTION_WRITE | SECTION_RELRO); return sect; } + /* If the SECTION_RETAIN bit doesn't match, return and switch + to a new section later. */ + if ((sect->common.flags & SECTION_RETAIN) + != (flags & SECTION_RETAIN)) + return sect; /* Sanity check user variables for flag changes. */ if (sect->named.decl != NULL && DECL_P (sect->named.decl) @@ -1852,7 +1857,7 @@ assemble_start_function (tree decl, const char *fnname) /* Switch to the correct text section for the start of the function. */ - switch_to_section (function_section (decl)); + switch_to_section (function_section (decl), decl); if (crtl->has_bb_partition && !hot_label_written) ASM_OUTPUT_LABEL (asm_out_file, crtl->subsections.hot_section_label); @@ -2348,7 +2353,7 @@ assemble_variable (tree decl, int top_level ATTRIBUTE_UNUSED, && (strcmp (sect->named.name, ".vtable_map_vars") == 0)) handle_vtv_comdat_section (sect, decl); else - switch_to_section (sect); + switch_to_section (sect, decl); if (align > BITS_PER_UNIT) ASM_OUTPUT_ALIGN (asm_out_file, floor_log2 (align / BITS_PER_UNIT)); assemble_variable_contents (decl, name, dont_output_data, @@ -7715,10 +7720,27 @@ output_section_asm_op (const void *directive) the current section is NEW_SECTION. */ void -switch_to_section (section *new_section) +switch_to_section (section *new_section, tree decl) { if (in_section == new_section) - return; + { + if (HAVE_GAS_SHF_GNU_RETAIN + && (new_section->common.flags & SECTION_NAMED) + && decl != nullptr + && (!!DECL_PRESERVE_P (decl) + != !!(new_section->common.flags & SECTION_RETAIN))) + { + /* If the SECTION_RETAIN bit doesn't match, switch to a new + section. */ + if (DECL_PRESERVE_P (decl)) + new_section->common.flags |= SECTION_RETAIN; + else + new_section->common.flags &= ~(SECTION_RETAIN + | SECTION_DECLARED); + } + else + return; + } if (new_section->common.flags & SECTION_FORGET) in_section = NULL;