[PATCH v7] Add retain attribute to place symbols in SHF_GNU_RETAIN section

[H.J. Lu]

On Thu, Feb 18, 2021 at 7:15 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Feb 18, 2021 at 4:01 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Thu, Feb 18, 2021 at 2:25 AM Richard Biener
> > <richard.guenther@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.tools@gmail.com> wrote:
> > > > >
> > > > > On Wed, Feb 17, 2021 at 12:14 PM Jakub Jelinek <jakub@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.