V3 [PATCH 0/2] Switch to a new section if the SECTION_RETAIN bit doesn't match

V3 [PATCH 0/2] Switch to a new section if the SECTION_RETAIN bit doesn't match

[H.J. Lu]

When SECTION_RETAIN is used, definitions marked with used attribute and
unmarked definitions are placed in a section with the same name.  Instead
of issue an error:

[hjl@gnu-cfl-2 gcc]$ /usr/gcc-11.0.0-x32/bin/gcc -S c.c -fdiagnostics-plain-output
c.c:2:49: error: ‘foo1’ causes a section type conflict with ‘foo2’
c.c:1:54: note: ‘foo2’ was declared here
[hjl@gnu-cfl-2 gcc]$

the first patch switches to a new section if the SECTION_RETAIN bit
doesn't match.  The second optional patch issues a warning:

[hjl@gnu-cfl-2 gcc]$ ./xgcc -B./ -S c.c
c.c:2:49: warning: ‘foo1’ without ‘used’ attribute and ‘foo2’ with ‘used’ attribute are placed in a section with the same name [-Wattributes]
    2 | const int __attribute__((section(".data.foo"))) foo1 = 1;
      |                                                 ^~~~
c.c:1:54: note: ‘foo2’ was declared here
    1 | const int __attribute__((used,section(".data.foo"))) foo2 = 2;
      |
[hjl@gnu-cfl-2 gcc]$

Changes from V2:

1. Add (new_section->common.flags & SECTION_NAMED) check since
SHF_GNU_RETAIN section must be named.
2. Move c-c++-common/attr-used-9.c to the fist patch since there are
no new warnings.
3. Check new warnings only for R_flag_in_section target.

H.J. Lu (2):
  Switch to a new section if the SECTION_RETAIN bit doesn't match
  Warn used and not used symbols in section with the same name

 gcc/output.h                             |  2 +-
 gcc/testsuite/c-c++-common/attr-used-5.c | 27 ++++++++++++++
 gcc/testsuite/c-c++-common/attr-used-6.c | 27 ++++++++++++++
 gcc/testsuite/c-c++-common/attr-used-7.c |  9 +++++
 gcc/testsuite/c-c++-common/attr-used-8.c |  9 +++++
 gcc/testsuite/c-c++-common/attr-used-9.c | 28 +++++++++++++++
 gcc/varasm.c                             | 46 +++++++++++++++++++++---
 7 files changed, 143 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

-- 
2.28.0

V3 [PATCH 1/2] Switch to a new section if the SECTION_RETAIN bit doesn't match

[H.J. Lu]

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

Re: V3 [PATCH 1/2] Switch to a new section if the SECTION_RETAIN bit doesn't match

[Vaseeharan Vinayagamoorthy]

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

Re: V3 [PATCH 1/2] Switch to a new section if the SECTION_RETAIN bit doesn't match

[Jeff Law]

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

Re: V3 [PATCH 1/2] Switch to a new section if the SECTION_RETAIN bit doesn't match

[H.J. Lu]

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
>


-- 
H.J.

V3 [PATCH 2/2] Warn used and not used symbols in section with the same name

[H.J. Lu]

When SECTION_RETAIN is used, issue a warning when a symbol without used
attribute and a symbol with used attribute are placed in the section with
the same name, like

int __attribute__((used,section(".data.foo"))) foo2 = 2;
int __attribute__((section(".data.foo"))) foo1 = 1;

since assembler will put them in different sections with the same section
name.

gcc/

	PR target/98146
	* varasm.c (switch_to_section): Warn when a symbol without used
	attribute and a symbol with used attribute are placed in the
	section with the same name.

gcc/testsuite/

	PR target/98146
	* c-c++-common/attr-used-5.c: Updated.
	* 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/testsuite/c-c++-common/attr-used-5.c |  1 +
 gcc/testsuite/c-c++-common/attr-used-6.c |  1 +
 gcc/testsuite/c-c++-common/attr-used-7.c |  1 +
 gcc/testsuite/c-c++-common/attr-used-8.c |  1 +
 gcc/varasm.c                             | 22 +++++++++++++++++++---
 5 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/gcc/testsuite/c-c++-common/attr-used-5.c b/gcc/testsuite/c-c++-common/attr-used-5.c
index 9fc0d3834e9..ba59326e452 100644
--- a/gcc/testsuite/c-c++-common/attr-used-5.c
+++ b/gcc/testsuite/c-c++-common/attr-used-5.c
@@ -10,6 +10,7 @@ extern struct dtv_slotinfo_list *list;
 
 static int __attribute__ ((section ("__libc_freeres_fn")))
 free_slotinfo (struct dtv_slotinfo_list **elemp)
+/* { dg-warning "'.*' without 'used' attribute and '.*' with 'used' attribute are placed in a section with the same name" "" { target R_flag_in_section } .-1 } */
 {
   if (!free_slotinfo (&(*elemp)->next))
     return 0;
diff --git a/gcc/testsuite/c-c++-common/attr-used-6.c b/gcc/testsuite/c-c++-common/attr-used-6.c
index 0cb82ade5a9..5d20f875bf0 100644
--- a/gcc/testsuite/c-c++-common/attr-used-6.c
+++ b/gcc/testsuite/c-c++-common/attr-used-6.c
@@ -18,6 +18,7 @@ free_slotinfo (struct dtv_slotinfo_list **elemp)
 
 __attribute__ ((section ("__libc_freeres_fn")))
 void free_mem (void)
+/* { dg-warning "'.*' without 'used' attribute and '.*' with 'used' attribute are placed in a section with the same name" "" { target R_flag_in_section } .-1 } */
 {
   free_slotinfo (&list);
 }
diff --git a/gcc/testsuite/c-c++-common/attr-used-7.c b/gcc/testsuite/c-c++-common/attr-used-7.c
index fba2706ffc1..75576bcabe5 100644
--- a/gcc/testsuite/c-c++-common/attr-used-7.c
+++ b/gcc/testsuite/c-c++-common/attr-used-7.c
@@ -3,6 +3,7 @@
 
 int __attribute__((used,section(".data.foo"))) foo2 = 2;
 int __attribute__((section(".data.foo"))) foo1 = 1;
+/* { dg-warning "'.*' without 'used' attribute and '.*' with 'used' attribute are placed in a section with the same name" "" { target R_flag_in_section } .-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
index 4da4aabe573..e4982db1044 100644
--- a/gcc/testsuite/c-c++-common/attr-used-8.c
+++ b/gcc/testsuite/c-c++-common/attr-used-8.c
@@ -2,6 +2,7 @@
 /* { dg-options "-Wall -O2" } */
 
 int __attribute__((section(".data.foo"))) foo1 = 1;
+/* { dg-warning "'.*' without 'used' attribute and '.*' with 'used' attribute are placed in a section with the same name" "" { target R_flag_in_section } .-1 } */
 int __attribute__((used,section(".data.foo"))) foo2 = 2;
 
 /* { dg-final { scan-assembler ".data.foo,\"aw\"" { target R_flag_in_section } } } */
diff --git a/gcc/varasm.c b/gcc/varasm.c
index c5ea29c4e4c..346b3bea890 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -7732,11 +7732,27 @@ switch_to_section (section *new_section, tree decl)
 	{
 	  /* If the SECTION_RETAIN bit doesn't match, switch to a new
 	     section.  */
+	  tree used_decl, no_used_decl;
+
 	  if (DECL_PRESERVE_P (decl))
-	    new_section->common.flags |= SECTION_RETAIN;
+	    {
+	      new_section->common.flags |= SECTION_RETAIN;
+	      used_decl = decl;
+	      no_used_decl = new_section->named.decl;
+	    }
 	  else
-	    new_section->common.flags &= ~(SECTION_RETAIN
-					   | SECTION_DECLARED);
+	    {
+	      new_section->common.flags &= ~(SECTION_RETAIN
+					     | SECTION_DECLARED);
+	      used_decl = new_section->named.decl;
+	      no_used_decl = decl;
+	    }
+	  warning (OPT_Wattributes,
+		   "%+qD without %<used%> attribute and %qD with "
+		   "%<used%> attribute are placed in a section with "
+		   "the same name", no_used_decl, used_decl);
+	  inform (DECL_SOURCE_LOCATION (used_decl),
+		  "%qD was declared here", used_decl);
 	}
       else
 	return;
-- 
2.28.0

Re: V3 [PATCH 0/2] Switch to a new section if the SECTION_RETAIN bit doesn't match

[Jim Wilson]

On Tue, Dec 8, 2020 at 4:51 AM H.J. Lu via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:

> When SECTION_RETAIN is used, definitions marked with used attribute and
> unmarked definitions are placed in a section with the same name.  Instead
> of issue an error:
>

Have you tested glibc builds with this patch?  I noticed yesterday that
your earlier patch had broken glibc builds, and was about to raise a bug
report for that.  I'm seeing

In file included from <command-line>:
gconv_db.c: In function 'free_mem':
gconv_db.c:831:18: error: 'free_mem' causes a section type conflict with
'free_derivation'
  831 | libc_freeres_fn (free_mem)
      |                  ^~~~~~~~
./../include/libc-symbols.h:316:15: note: in definition of macro
'libc_freeres_fn'
  316 |   static void name (void)
      |               ^~~~
gconv_db.c:174:1: note: 'free_derivation' was declared here
  174 | free_derivation (void *p)
      | ^~~~~~~~~~~~~~~

This is because free_derivation and free_mem are in the same section, but
free_mem has attribute used and free_derivation does not.

This patch is changing the error to a warning, which I think solves the
problem unless --enable-werror is used, which is probably good enough.  We
could maybe decide that what glibc is doing is wrong and fix glibc to use
different section names or mark both functions as attribute used.

Jim

Re: V3 [PATCH 0/2] Switch to a new section if the SECTION_RETAIN bit doesn't match

[H.J. Lu]

On Wed, Dec 9, 2020 at 6:08 PM Jim Wilson <jimw@sifive.com> wrote:
>
> On Tue, Dec 8, 2020 at 4:51 AM H.J. Lu via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>
>> When SECTION_RETAIN is used, definitions marked with used attribute and
>> unmarked definitions are placed in a section with the same name.  Instead
>> of issue an error:
>
>
> Have you tested glibc builds with this patch?  I noticed yesterday that your earlier patch had broken glibc builds, and was about to raise a bug report for that.  I'm seeing
>
> In file included from <command-line>:
> gconv_db.c: In function 'free_mem':
> gconv_db.c:831:18: error: 'free_mem' causes a section type conflict with 'free_derivation'
>   831 | libc_freeres_fn (free_mem)
>       |                  ^~~~~~~~
> ./../include/libc-symbols.h:316:15: note: in definition of macro 'libc_freeres_fn'
>   316 |   static void name (void)
>       |               ^~~~
> gconv_db.c:174:1: note: 'free_derivation' was declared here
>   174 | free_derivation (void *p)
>       | ^~~~~~~~~~~~~~~
>
> This is because free_derivation and free_mem are in the same section, but free_mem has attribute used and free_derivation does not.
>
> This patch is changing the error to a warning, which I think solves the problem unless --enable-werror is used, which is probably good enough.  We could maybe decide that what glibc is doing is wrong and fix glibc to use different section names or mark both functions as attribute used.
>

I tested it with glibc build.  Glibc build issue is the reason I
didn't combine 2 patches into one.
If GCC does issue a warning, which it should, we will change glibc.
If we decide not to issue
a warning, there is no need to change glibc.

BTW, I believe glibc should be changed.

-- 
H.J.

Re: V3 [PATCH 0/2] Switch to a new section if the SECTION_RETAIN bit doesn't match

[Jim Wilson]

On Wed, Dec 9, 2020 at 6:14 PM H.J. Lu <hjl.tools@gmail.com> wrote:

> I tested it with glibc build.  Glibc build issue is the reason I
> didn't combine 2 patches into one.
> If GCC does issue a warning, which it should, we will change glibc.
>

OK.  Thanks.  Then I won't worry about this glibc for now.

Jim

PING^1: V3 [PATCH 0/2] Switch to a new section if the SECTION_RETAIN bit doesn't match

[H.J. Lu]

On Tue, Dec 8, 2020 at 4:51 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> When SECTION_RETAIN is used, definitions marked with used attribute and
> unmarked definitions are placed in a section with the same name.  Instead
> of issue an error:
>
> [hjl@gnu-cfl-2 gcc]$ /usr/gcc-11.0.0-x32/bin/gcc -S c.c -fdiagnostics-plain-output
> c.c:2:49: error: ‘foo1’ causes a section type conflict with ‘foo2’
> c.c:1:54: note: ‘foo2’ was declared here
> [hjl@gnu-cfl-2 gcc]$
>
> the first patch switches to a new section if the SECTION_RETAIN bit
> doesn't match.  The second optional patch issues a warning:
>
> [hjl@gnu-cfl-2 gcc]$ ./xgcc -B./ -S c.c
> c.c:2:49: warning: ‘foo1’ without ‘used’ attribute and ‘foo2’ with ‘used’ attribute are placed in a section with the same name [-Wattributes]
>     2 | const int __attribute__((section(".data.foo"))) foo1 = 1;
>       |                                                 ^~~~
> c.c:1:54: note: ‘foo2’ was declared here
>     1 | const int __attribute__((used,section(".data.foo"))) foo2 = 2;
>       |
> [hjl@gnu-cfl-2 gcc]$
>
> Changes from V2:
>
> 1. Add (new_section->common.flags & SECTION_NAMED) check since
> SHF_GNU_RETAIN section must be named.
> 2. Move c-c++-common/attr-used-9.c to the fist patch since there are
> no new warnings.
> 3. Check new warnings only for R_flag_in_section target.
>
> H.J. Lu (2):
>   Switch to a new section if the SECTION_RETAIN bit doesn't match
>   Warn used and not used symbols in section with the same name
>
>  gcc/output.h                             |  2 +-
>  gcc/testsuite/c-c++-common/attr-used-5.c | 27 ++++++++++++++
>  gcc/testsuite/c-c++-common/attr-used-6.c | 27 ++++++++++++++
>  gcc/testsuite/c-c++-common/attr-used-7.c |  9 +++++
>  gcc/testsuite/c-c++-common/attr-used-8.c |  9 +++++
>  gcc/testsuite/c-c++-common/attr-used-9.c | 28 +++++++++++++++
>  gcc/varasm.c                             | 46 +++++++++++++++++++++---
>  7 files changed, 143 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
>

PING.

-- 
H.J.