[Bug c/99587] New: warning: ‘retain’ attribute ignored while __has_attribute(retain) is true

[Bug c/99587] New: warning: ‘retain’ attribute ignored while __has_attribute(retain) is true

[i at maskray dot me]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99587

            Bug ID: 99587
           Summary: warning: ‘retain’ attribute ignored while
                    __has_attribute(retain) is true
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: i at maskray dot me
  Target Milestone: ---

If configure-time ld does not support SHF_GNU_RETAIN,  __has_attribute(retain)
may be true while using it will cause a warning.

% cat x.c
#if defined(__has_attribute) && __has_attribute(retain)
__attribute__((used, retain)) int a;
#endif
% ~/Dev/gcc/out/release/gcc/xgcc -B ~/Dev/gcc/out/release/gcc -c x.c
x.c:1:1: warning: ‘retain’ attribute ignored [-Wattributes]
    1 | __attribute__((used, retain)) int a;
      | ^~~~~~~~~~~~~
% ~/Dev/gcc/out/release/gcc/xgcc --version                          
xgcc (GCC) 11.0.1 20210313 (experimental)
...


__has_attribute(retain) should return 0 in this case.

[Bug c/99587] warning: ‘retain’ attribute ignored while __has_attribute(retain) is 1

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99587

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-03-15
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
The attribute is ignored because 'a' is exported, so it's retained anyway (but
not marked as SHF_GNU_RETAIN).  Not sure what the desired semantics of the
attribute are here.

static tree
handle_retain_attribute (tree *pnode, tree name, tree ARG_UNUSED (args),
                         int ARG_UNUSED (flags), bool *no_add_attrs)
{
  tree node = *pnode;

  if (SUPPORTS_SHF_GNU_RETAIN
      && (TREE_CODE (node) == FUNCTION_DECL
          || (VAR_P (node) && TREE_STATIC (node))))
    ;
  else
    {
      warning (OPT_Wattributes, "%qE attribute ignored", name);

in particular 'a' is not TREE_STATIC.  Maybe !DECL_EXTERNAL was intended here
to ignore it on

extern int a;

?

[Bug c/99587] warning: ‘retain’ attribute ignored while __has_attribute(retain) is 1

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99587

--- Comment #2 from Florian Weimer <fw at gcc dot gnu.org> ---
The problem is that if GCC is not configured for SHF_GNU_RETAIN,
__has_attribute (retain) should not be true.

That is, __has_attribute needs to reflect the actual attribute support status,
and not what happens to be registered as attributes in the GCC codebase. 
__has_builtin has the same problem, see bug 96952 for an example.

[Bug c/99587] warning: ‘retain’ attribute ignored while __has_attribute(retain) is 1

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99587

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
But we currently have no easy way how to know that.  The rejection of the
attribute as unsupported is done in various places and can depend on many
conditions, in this case it is just a configure thing (plus it is ignored on
entities other than variables and function declarations), but for other
attributes it can depend on command line options, and that can even be changed
on a function by function basis.

__has_attribute as currently implemented just answers the question whether gcc
knows about the attribute (has it registered).

[Bug c/99587] warning: ‘retain’ attribute ignored while __has_attribute(retain) is 1

[fw at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99587

--- Comment #4 from Florian Weimer <fw at gcc dot gnu.org> ---
For retain, something along these lines might work:

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index c1f652d1dc9..cdae464ab8a 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -329,8 +329,10 @@ const struct attribute_spec c_common_attribute_table[] =
                              handle_used_attribute, NULL },
   { "unused",                 0, 0, false, false, false, false,
                              handle_unused_attribute, NULL },
+#if SUPPORTS_SHF_GNU_RETAIN
   { "retain",                 0, 0, true,  false, false, false,
                              handle_retain_attribute, NULL },
+#endif
   { "externally_visible",     0, 0, true,  false, false, false,
                              handle_externally_visible_attribute, NULL },
   { "no_reorder",            0, 0, true, false, false, false,

In other cases, it's more difficult because those are subtarget-dependent.

It's not particularly useful to know that a particular source code base of GCC
knows about the attribute in principle, if built for the right target and with
the right binutils/glibc versions etc. A programmer can already use a version
check for that. __has_attribute and __has_builtin are only useful if they
reflect the current GCC build and its target flags.

[Bug c/99587] warning: ‘retain’ attribute ignored while __has_attribute(retain) is 1

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99587

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Florian Weimer from comment #4)
> For retain, something along these lines might work:
> 
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index c1f652d1dc9..cdae464ab8a 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -329,8 +329,10 @@ const struct attribute_spec c_common_attribute_table[] =
>                               handle_used_attribute, NULL },
>    { "unused",                 0, 0, false, false, false, false,
>                               handle_unused_attribute, NULL },
> +#if SUPPORTS_SHF_GNU_RETAIN
>    { "retain",                 0, 0, true,  false, false, false,
>                               handle_retain_attribute, NULL },
> +#endif
>    { "externally_visible",     0, 0, true,  false, false, false,
>                               handle_externally_visible_attribute, NULL },
>    { "no_reorder",            0, 0, true, false, false, false,
> 
> In other cases, it's more difficult because those are subtarget-dependent.

Doing the above would "fix" __has_attribute, but on the other side would mean
the compiler would not know how many and what kind of operands the attribute
has, whether it is for function declarations, other declarations, types or what
etc., so for invalid code it would have inconsistent diagnostics.

[Bug c/99587] warning: ‘retain’ attribute ignored while __has_attribute(retain) is 1

[i at maskray dot me]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99587

--- Comment #6 from Fangrui Song <i at maskray dot me> ---
(In reply to Jakub Jelinek from comment #5)
> (In reply to Florian Weimer from comment #4)
> > For retain, something along these lines might work:
> > 
> > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> > index c1f652d1dc9..cdae464ab8a 100644
> > --- a/gcc/c-family/c-attribs.c
> > +++ b/gcc/c-family/c-attribs.c
> > @@ -329,8 +329,10 @@ const struct attribute_spec c_common_attribute_table[] =
> >                               handle_used_attribute, NULL },
> >    { "unused",                 0, 0, false, false, false, false,
> >                               handle_unused_attribute, NULL },
> > +#if SUPPORTS_SHF_GNU_RETAIN
> >    { "retain",                 0, 0, true,  false, false, false,
> >                               handle_retain_attribute, NULL },
> > +#endif
> >    { "externally_visible",     0, 0, true,  false, false, false,
> >                               handle_externally_visible_attribute, NULL },
> >    { "no_reorder",            0, 0, true, false, false, false,
> > 
> > In other cases, it's more difficult because those are subtarget-dependent.
> 
> Doing the above would "fix" __has_attribute, but on the other side would mean
> the compiler would not know how many and what kind of operands the attribute
> has, whether it is for function declarations, other declarations, types or
> what
> etc., so for invalid code it would have inconsistent diagnostics.

Are you willing to properly fix it? :)

I implemented the attribute on clang (https://reviews.llvm.org/D97447).
__has_attribute(retain) is always 1 and there is no ignored diagnostic,
regardless of the target (even if non-ELF), and __has_attribute(retain) works
in assembly mode as well. This is intentional so that: with bleeding-edge
toolchain, non-ELF targets don't need macros to decide whether 'retain' should
be added.


Ultimately, I want the glibc static linking problem with ld -z start-stop-gc
fixed
https://sourceware.org/pipermail/libc-alpha/2021-March/123833.html
(glibc has -Wattributes, so __has_attribute(retain)=1 && "warning: ‘retain’
attribute ignored" can cause some inconvenience.)

And I hope eventually ld -z start-stop-gc can be the default.