On Thu, Feb 18, 2021 at 7:15 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
> On Thu, Feb 18, 2021 at 4:01 PM H.J. Lu <hjl.to...@gmail.com> wrote:
> >
> > On Thu, Feb 18, 2021 at 2:25 AM Richard Biener
> > <richard.guent...@gmail.com> wrote:
> > >
> > > On Thu, Feb 18, 2021 at 5:15 AM H.J. Lu via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > > >
> > > > On Wed, Feb 17, 2021 at 12:50 PM H.J. Lu <hjl.to...@gmail.com> wrote:
> > > > >
> > > > > On Wed, Feb 17, 2021 at 12:14 PM Jakub Jelinek <ja...@redhat.com>
> > > > > wrote:
> > > > > >
> > > > > > On Wed, Feb 17, 2021 at 12:04:34PM -0800, H.J. Lu wrote:> >
> > > > > > > + /* For -fretain-used-symbol, a "used" attribute also implies
> > > > > > > "retain". */
> > > > > >
> > > > > > s/-symbol/-symbols/
> > > > >
> > > > > Fixed.
> > > > >
> > > > > > > + if (flag_retain_used_symbols
> > > > > > > + && attributes
> > > > > > > + && (TREE_CODE (*node) == FUNCTION_DECL
> > > > > > > + || (VAR_P (*node) && TREE_STATIC (*node))
> > > > > > > + || (TREE_CODE (*node) == TYPE_DECL))
> > > > > >
> > > > > > What will "retain" do on TYPE_DECLs? It only makes sense to me
> > > > > > for FUNCTION_DECL or non-automatic VAR_DECLs, unless for types
> > > > > > it means the types with that result in vars with those types
> > > > > > automatically
> > > > > > getting "retain", but there is no code for that and I don't see
> > > > > > "used"
> > > > > > handled that way.
> > > > >
> > > > > Fixed.
> > > > >
> > > > > > > + if (SUPPORTS_SHF_GNU_RETAIN
> > > > > > > + && (TREE_CODE (node) == FUNCTION_DECL
> > > > > > > + || (VAR_P (node) && TREE_STATIC (node))
> > > > > > > + || (TREE_CODE (node) == TYPE_DECL)))
> > > > > >
> > > > > > Likewise.
> > > > >
> > > > > Fixed.
> > > > >
> > > > > > > + ;
> > > > > > > + else
> > > > > > > + {
> > > > > > > + warning (OPT_Wattributes, "%qE attribute ignored", name);
> > > > > > > + *no_add_attrs = true;
> > > > > > > + }
> > > > > > > +
> > > > > > > + return NULL_TREE;
> > > > > > > +}
> > > > > > > +
> > > > > > > /* Handle a "externally_visible" attribute; arguments as in
> > > > > > > struct attribute_spec.handler. */
> > > > > > >
> > > > > > > diff --git a/gcc/common.opt b/gcc/common.opt
> > > > > > > index c75dd36843e..70842481d4d 100644
> > > > > > > --- a/gcc/common.opt
> > > > > > > +++ b/gcc/common.opt
> > > > > > > @@ -2404,6 +2404,10 @@ frerun-loop-opt
> > > > > > > Common Ignore
> > > > > > > Does nothing. Preserved for backward compatibility.
> > > > > > >
> > > > > > > +fretain-used-symbols
> > > > > > > +Common Var(flag_retain_used_symbols)
> > > > > > > +Make the used attribute to imply the retain attribute if
> > > > > > > supported.
> > > > > >
> > > > > > English is not my native language, but I think the "to " doesn't
> > > > > > belong
> > > > > > here.
> > > > >
> > > > > Fixed.
> > > > >
> > > > > > > +@item -fretain-used-symbols
> > > > > > > +@opindex fretain-used-symbols
> > > > > > > +On systems with recent GNU assembler and linker, the compiler
> > > > > > > makes
> > > > > >
> > > > > > I think we should avoid recent here, new/old/recent etc. are
> > > > > > relative terms.
> > > > > > Either use exact version (is it 2.36 or later) or say GNU assembler
> > > > > > and
> > > > > > linker that supports the @code{.retain} directive or something
> > > > > > similar.
> > > > >
> > > > > Fixed.
> > > > >
> > > > > > > flags |= SECTION_NAMED;
> > > > > > > - if (SUPPORTS_SHF_GNU_RETAIN
> > > > > > > - && decl != nullptr
> > > > > > > + if (decl != nullptr
> > > > > > > && DECL_P (decl)
> > > > > > > - && DECL_PRESERVE_P (decl))
> > > > > > > + && DECL_PRESERVE_P (decl)
> > > > > > > + && lookup_attribute ("retain", DECL_ATTRIBUTES (decl)))
> > > > > > > flags |= SECTION_RETAIN;
> > > > > > > if (*slot == NULL)
> > > > > > > {
> > > > > >
> > > > > > I think none of the varasm.c code should use DECL_PRESERVE_P when
> > > > > > it means
> > > > > > retain, just lookup_attribute.
> > > > >
> > > > > Fixed.
> > > > >
> > > > > > > @@ -487,7 +487,8 @@ resolve_unique_section (tree decl, int reloc
> > > > > > > ATTRIBUTE_UNUSED,
> > > > > > > if (DECL_SECTION_NAME (decl) == NULL
> > > > > > > && targetm_common.have_named_sections
> > > > > > > && (flag_function_or_data_sections
> > > > > > > - || (SUPPORTS_SHF_GNU_RETAIN && DECL_PRESERVE_P (decl))
> > > > > > > + || (DECL_PRESERVE_P (decl)
> > > > > > > + && lookup_attribute ("retain", DECL_ATTRIBUTES
> > > > > > > (decl)))
> > > > > > > || DECL_COMDAT_GROUP (decl)))
> > > > > > > {
> > > > > > > targetm.asm_out.unique_section (decl, reloc);
> > > > > > > @@ -1227,7 +1228,8 @@ get_variable_section (tree decl, bool
> > > > > > > prefer_noswitch_p)
> > > > > > > vnode->get_constructor ();
> > > > > > >
> > > > > > > if (DECL_COMMON (decl)
> > > > > > > - && !(SUPPORTS_SHF_GNU_RETAIN && DECL_PRESERVE_P (decl)))
> > > > > > > + && !(DECL_PRESERVE_P (decl)
> > > > > > > + && lookup_attribute ("retain", DECL_ATTRIBUTES (decl))))
> > > > > > > {
> > > > > > > /* If the decl has been given an explicit section name, or
> > > > > > > it resides
> > > > > > > in a non-generic address space, then it isn't common, and
> > > > > > > shouldn't
> > > > > > > @@ -7761,12 +7763,14 @@ switch_to_section (section *new_section,
> > > > > > > tree decl)
> > > > > > > {
> > > > > > > if (in_section == new_section)
> > > > > > > {
> > > > > > > - if (SUPPORTS_SHF_GNU_RETAIN
> > > > > > > - && (new_section->common.flags & SECTION_NAMED)
> > > > > > > + if ((new_section->common.flags & SECTION_NAMED)
> > > > > > > && decl != nullptr
> > > > > > > && DECL_P (decl)
> > > > > > > && (!!DECL_PRESERVE_P (decl)
> > > > > > > - != !!(new_section->common.flags & SECTION_RETAIN)))
> > > > > > > + != !!(new_section->common.flags & SECTION_RETAIN))
> > > > > > > + && (lookup_attribute ("retain",
> > > > > > > + DECL_ATTRIBUTES
> > > > > > > (new_section->named.decl))
> > > > > > > + || lookup_attribute ("retain", DECL_ATTRIBUTES
> > > > > > > (decl))))
> > > > > > > {
> > > > > > > /* If the SECTION_RETAIN bit doesn't match, switch to a
> > > > > > > new
> > > > > > > section. */
> > > > > > > --
> > > > > > > 2.29.2
> > > > > > >
> > > > >
> > > > > Here is the updated patch.
> > > > >
> > > >
> > > > Here is the updated patch. Tested on Linux/x86-64 with binutils
> > > > master branch. OK for master?
> > >
> > > If you can split it up into adding the 'retain' attribute and the
> > > new -fretain-used-symbols option (which I think should not be
> > > added) I'll approve the 'retain' attribute parts plus a patch to
> > > no longer apply SHF_GNU_RETAIN for 'used'.
> >
> > Done.
> >
> > > 'used' and 'retain' are two different things and that's how it should
> > > remain.
> >
> > Here is the patch. OK for master?
>
> LGTM. Please leave others some hours to comment and spot any issues I did
> not.
>
>
I am checking it in in the next hour.
--
H.J.
