Hi H.J.,
On Thu, Dec 03, 2020 at 04:06:51PM -0800, H.J. Lu via Gcc-patches wrote:
> When definitions marked with used attribute and unmarked definitions are
> placed in the same section, switch to a new section if the SECTION_RETAIN
> bit doesn't match.
GAS doesn't create separate sections for
.section .data.foo,"aw"
.section .data.foo,"awR"
A single .data.foo section, with SHF_GNU_RETAIN set, is output to the
object file.
This is because the addition of SHF_GNU_RETAIN support to the "used"
attribute was not supposed to be modifying the layout of sections in the
object file.
Now we are placing "used" decls in a new, unique section (when they
don't already have a unique section), I suppose it is fine to have two
separate sections for retained/non-retained .data.foo in the object
file.
It does feel like a bit of a kludge that we end up with
[ 4] .data.foo PROGBITS 0000000000000000 000040 000006 00 WA 0 0 1
[ 5] .data.foo PROGBITS 0000000000000000 000046 000006 00 WAR 0 0 1
but it is beneficial to the user, since linker garbage collection
will remove more unused parts of their program.
Your commit "2be7ea19b3 Add SEC_ASSEMBLER_SHF_MASK" on x86-binutils
implements this, it just needs some fix ups to apply cleanly.
Note that when .section is used without any section attributes
specified, then the non-SHF_GNU_RETAIN section is switched to.
I would expect it to switch to the most recently switched to section.
After applying your above SEC_ASSEMBLER_SHF_MASK commit, the following
assembly code:
.section .data.foo,"aw"
.word 0
.section .data.foo
.word 0
.section .data.foo,"awR"
.word 0
.section .data.foo
.word 0
results in the following section layout
sh_size
[ 4] .data.foo PROGBITS 0000000000000000 000040 000006 00 WA 0 0 1
[ 5] .data.foo PROGBITS 0000000000000000 000046 000002 00 WAR 0 0 1
As you can see, the final ".section .data.foo" instance has been
associated with the non-SHF_GNU_RETAIN section.
To align with this GCC patch, the final ".section .data.foo" directive
should in fact switch to the most recently switched to section, which
would be the SHF_GNU_RETAIN instance of .data.foo.
I've included some other comments inline with the patch
Thanks,
Jozef
>
> gcc/
>
> PR other/98121
> * 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 other/98121
> * 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.
> ---
> 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/varasm.c | 28 ++++++++++++++++++++----
> 6 files changed, 93 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
>
> 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 } } } */
I think a new copy of the following test should be made to check that a
static free_mem will be removed by GCC, even if another decl in the same
section is marked used. This clarifies behavior Florian was concerned
about in PR/98121.
> 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..4526a692ee4
> --- /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")))
If you make this "static void free_mem" and...
> +void free_mem (void)
> +{
> + free_slotinfo (&list);
> +}
> +
... change this to 'scan-assembler-not "free_mem"'.
> +/* { dg-final { scan-assembler "__libc_freeres_fn\n" } } */
> +/* { 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..c8d65f65033
> --- /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\n" { target R_flag_in_section } }
> } */
> +/* { dg-final { scan-assembler ".data.foo,\"awR\"" { target
> R_flag_in_section } } } */
> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index 961d2d6fe3b..7271705198c 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -339,6 +339,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)
> @@ -1849,7 +1854,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);
>
> @@ -2345,7 +2350,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,
> @@ -7712,10 +7717,25 @@ 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
> + && 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;
> + }
> + else
> + return;
> + }
>
> if (new_section->common.flags & SECTION_FORGET)
> in_section = NULL;
> --
> 2.28.0
>
