Bug 99587 - warning: ‘retain’ attribute ignored while __has_attribute(retain) is 1
Summary: warning: ‘retain’ attribute ignored while __has_attribute(retain) is 1
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 11.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks:
 
Reported: 2021-03-15 01:23 UTC by Fangrui Song
Modified: 2024-03-14 00:59 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2021-03-15 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Fangrui Song 2021-03-15 01:23:09 UTC
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.
Comment 1 Richard Biener 2021-03-15 08:51:21 UTC
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;

?
Comment 2 Florian Weimer 2021-03-15 09:06:10 UTC
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.
Comment 3 Jakub Jelinek 2021-03-15 10:10:26 UTC
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).
Comment 4 Florian Weimer 2021-03-15 10:18:55 UTC
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.
Comment 5 Jakub Jelinek 2021-03-15 10:26:01 UTC
(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.
Comment 6 Fangrui Song 2021-03-16 21:30:46 UTC
(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.