Bug 98121 - [11 Regression] __attribute__ ((used)) should not imply SHF_RETAIN_SECTION
Summary: [11 Regression] __attribute__ ((used)) should not imply SHF_RETAIN_SECTION
Status: RESOLVED WONTFIX
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 11.0
: P3 normal
Target Milestone: 11.0
Assignee: Not yet assigned to anyone
URL: https://gcc.gnu.org/pipermail/gcc-pat...
Keywords: patch
Depends on:
Blocks:
 
Reported: 2020-12-03 13:35 UTC by Florian Weimer
Modified: 2020-12-04 15:01 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-12-03 00:00:00


Attachments
A patch (2.00 KB, patch)
2020-12-03 19:15 UTC, H.J. Lu
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Weimer 2020-12-03 13:35:40 UTC
The used attribute has already a defined meaning. I think it's wrong to cause it to change section attributes because people use it to interface with assembler code, so section flags conflicts are rather likely.

Using a separate attribute would also avoid the need for the configure check in GCC.

This is related to this commit (note the date is off, it's actually GCC 11 only):

commit 6fbec038f7a7ddf29f074943611b53210d17c40c
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Mon Feb 3 11:55:43 2020 -0800

    Use SHF_GNU_RETAIN to preserve symbol definitions
Comment 1 Jozef Lawrynowicz 2020-12-03 14:36:36 UTC
The feedback after I originally submitted a patch that implements a "retain" attribute to apply SHF_GNU_RETIAN, was that "used" should instead apply SHF_GNU_RETAIN:

https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557097.html

The discussions went on about other aspects of the implementation in other threads, but there were no objections to the idea of "used" applying SHF_GNU_RETAIN.

I think the logic is sound that a declaration marked with the "used" attribute should not be removed by the compiler, or garbage collected by the linker. Basically, if you mark a declaration as "used", it should be retained in the final program, even if it was not referenced.

The issue lies in the fact that the "used" attribute has been around for so long, that the change in behavior has the potential to affect existing applications that make use of it.

However, the new behavior is only enabled when using the latest GCC and latest Binutils. Some change in behavior can be expected when using the latest versions of the tools. This new behavior should really be documented in the release notes though.

I didn't want to have "used" change section attributes in assembler directives, the original plan was to use a separate assembler ".retain" directive to apply SHF_GNU_RETAIN. This would mean we could leave .section assembler directives for a particular "used" declaration untouched, and just emit a ".retain <symbol_name>" directive. The assembler therefore handles the application of the SHF_GNU_RETAIN flag to a particular section, instead of forcing the compiler to have to make these additional considerations about section attributes.

I even did a patch for that, but there was staunch objection from H.J. on this, on multiple occasions, I realized we weren't going to get anywhere:

https://gcc.gnu.org/pipermail/gcc-patches/2020-November/557931.html
https://gcc.gnu.org/pipermail/gcc-patches/2020-November/558400.html
https://sourceware.org/pipermail/binutils/2020-November/114077.html

I would be more than happy to add the .retain attribute to Binutils, but we would need a Binutils global maintainer to approve that, and no one gave the OK for my original ".retain <symbol_name>" patch the first time round. I could then change the GCC implementation to use the ".retain" directive, which would ensure that section attributes in assembler code output by GCC are not contaminated with the "R" flag when the "used" attribute is applied.

Thanks,
Jozef
Comment 2 H.J. Lu 2020-12-03 14:55:37 UTC
The issue here is that what should happen when definitions marked with used
attribute and unmarked definitions are put in the same section. It has nothing
to do with .retain.
Comment 3 H.J. Lu 2020-12-03 14:58:31 UTC
(In reply to H.J. Lu from comment #2)
> The issue here is that what should happen when definitions marked with used
> attribute and unmarked definitions are put in the same section. It has
> nothing
> to do with .retain.

If we cant to use SHF_GNU_RETAIN on used attribute, compiler can do

1. Issue an error.
2. issue a warning and mark the unmarked definition as used.
3. Silently mark the unmarked definition as used.
Comment 4 Jozef Lawrynowicz 2020-12-03 16:42:30 UTC
(In reply to H.J. Lu from comment #2)
> The issue here is that what should happen when definitions marked with used
> attribute and unmarked definitions are put in the same section. It has
> nothing
> to do with .retain.

This bug report doesn't mention that. It looks to me that Florian was looking to discuss the conceptual issue of whether the "used" attribute should apply SHF_GNU_RETAIN.

I assume you're referring to the glibc bug 27002 with though.

(In reply to H.J. Lu from comment #3)
> If we cant to use SHF_GNU_RETAIN on used attribute, compiler can do
> 
> 1. Issue an error.
> 2. issue a warning and mark the unmarked definition as used.
> 3. Silently mark the unmarked definition as used.

GAS merges the "R" flag state in .section declarations, silently, and with logical OR, and GCC should do the same. So if you have:

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

.data.foo should have SECTION_RETAIN set within GCC. The addition of the "used" attribute to the second declaration of section(".data.foo") should not cause any warning/error messages to be emitted either. So option 3 from above.

Just need to do something similar to what is already done for SECTION_NOTYPE in varasm.c:get_section.
Comment 5 H.J. Lu 2020-12-03 16:45:47 UTC
[hjl@gnu-cfl-2 tmp]$ cat x.c
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);
}
[hjl@gnu-cfl-2 tmp]$ /usr/gcc-11.0.0-x32/bin/gcc -S -O2 x.c -fPIC
x.c: In function ‘free_mem’:
x.c:17:13: error: ‘free_mem’ causes a section type conflict with ‘free_slotinfo.isra’
   17 | static void free_mem (void)
      |             ^~~~~~~~
x.c:9:1: note: ‘free_slotinfo.isra’ was declared here
    9 | free_slotinfo (struct dtv_slotinfo_list **elemp)
      | ^~~~~~~~~~~~~
[hjl@gnu-cfl-2 tmp]$
Comment 6 Florian Weimer 2020-12-03 16:53:34 UTC
(In reply to Jozef Lawrynowicz from comment #4)
> GAS merges the "R" flag state in .section declarations, silently, and with
> logical OR, and GCC should do the same. So if you have:
> 
> int __attribute__((section(".data.foo"))) foo1 = 1;
> int __attribute__((used,section(".data.foo"))) foo2 = 2;
> 
> .data.foo should have SECTION_RETAIN set within GCC. The addition of the
> "used" attribute to the second declaration of section(".data.foo") should
> not cause any warning/error messages to be emitted either. So option 3 from
> above.
> 
> Just need to do something similar to what is already done for SECTION_NOTYPE
> in varasm.c:get_section.

I don't now the details, but I think foo1 should not implicitly be marked as “used”. The difference matters for static functions, for example. GCC should still drop them if they are not referenced, even if they are located in a retained section.  So free_slotinfo in comment 5.
Comment 7 Jozef Lawrynowicz 2020-12-03 17:05:54 UTC
(In reply to Florian Weimer from comment #6)
> (In reply to Jozef Lawrynowicz from comment #4)
> > GAS merges the "R" flag state in .section declarations, silently, and with
> > logical OR, and GCC should do the same. So if you have:
> > 
> > int __attribute__((section(".data.foo"))) foo1 = 1;
> > int __attribute__((used,section(".data.foo"))) foo2 = 2;
> > 
> > .data.foo should have SECTION_RETAIN set within GCC. The addition of the
> > "used" attribute to the second declaration of section(".data.foo") should
> > not cause any warning/error messages to be emitted either. So option 3 from
> > above.
> > 
> > Just need to do something similar to what is already done for SECTION_NOTYPE
> > in varasm.c:get_section.
> 
> I don't now the details, but I think foo1 should not implicitly be marked as
> “used”. The difference matters for static functions, for example. GCC should
> still drop them if they are not referenced, even if they are located in a
> retained section.  So free_slotinfo in comment 5.

With the suggested approach, GCC would still drop foo1 if it is not used, since it is only the section being marked with SECTION_RETAIN, and this only affects whether the "R" flag is output in the .section directive for the decl.

GCC will only keep an unreferenced decl if it has DECL_PRESERVE_P and we haven't changed that with the "used" applies SHF_GNU_RETAIN functionality.

If foo1 somehow makes it into the assembler code, it will be kept, even if it is not used, since we lose the ability to remove individual decls once they are put in sections.
Comment 8 Jozef Lawrynowicz 2020-12-03 17:10:09 UTC
(In reply to Jozef Lawrynowicz from comment #7)
> (In reply to Florian Weimer from comment #6)
> > (In reply to Jozef Lawrynowicz from comment #4)
> > > GAS merges the "R" flag state in .section declarations, silently, and with
> > > logical OR, and GCC should do the same. So if you have:
> > > 
> > > int __attribute__((section(".data.foo"))) foo1 = 1;
> > > int __attribute__((used,section(".data.foo"))) foo2 = 2;
> > > 
> > > .data.foo should have SECTION_RETAIN set within GCC. The addition of the
> > > "used" attribute to the second declaration of section(".data.foo") should
> > > not cause any warning/error messages to be emitted either. So option 3 from
> > > above.
> > > 
> > > Just need to do something similar to what is already done for SECTION_NOTYPE
> > > in varasm.c:get_section.
> > 
> > I don't now the details, but I think foo1 should not implicitly be marked as
> > “used”. The difference matters for static functions, for example. GCC should
> > still drop them if they are not referenced, even if they are located in a
> > retained section.  So free_slotinfo in comment 5.
> 
> With the suggested approach, GCC would still drop foo1 if it is not used,
> since it is only the section being marked with SECTION_RETAIN, and this only
> affects whether the "R" flag is output in the .section directive for the
> decl.
> 
> GCC will only keep an unreferenced decl if it has DECL_PRESERVE_P and we
> haven't changed that with the "used" applies SHF_GNU_RETAIN functionality.
> 
> If foo1 somehow makes it into the assembler code, it will be kept, even if
> it is not used, since we lose the ability to remove individual decls once
> they are put in sections.

Sorry, in my foo1/foo2 example they are not static so will not be dropped by GCC.

For the free_slotinfo example, it would be dropped by GCC if unused, even though another function with the same section was marked "used" and has SECTION_RETAIN.
Comment 9 H.J. Lu 2020-12-03 19:15:09 UTC
Created attachment 49675 [details]
A patch
Comment 10 H.J. Lu 2020-12-04 00:08:53 UTC
A patch set is posted at

https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561075.html
Comment 11 Jozef Lawrynowicz 2020-12-04 15:01:07 UTC
I'm going to resolve this as WONTFIX since we're not going to change the fact that the "used" attribute applies SHF_GNU_RETAIN.

There are no section flag conflicts when a declaration that has the "used" attribute applied interfaces with a section defined in assembler code. The assembler will keep sections with different SHF_GNU_RETAIN states separate.

There was a GCC bug causing section flag conflicts in one specific situation, that is tracked in PR target/98146.