Re: [PATCH] add support for GCC 9 attribute copy

[Martin Sebor <msebor at gmail dot com>]

On 11/12/2018 04:10 PM, Joseph Myers wrote:
> On Mon, 12 Nov 2018, Martin Sebor wrote:
>
> > > Could you look at the powerpc soft-float, s390x and mips failures, which
> > > look somewhat different (and thus could indicate issues with the details
> > > of how this warning / attribute are specified, as opposed to simply
> > > needing a few more copy attributes somewhere)?
> >
> > Do you by chance have an easy way to get the preprocessed sources
> > for these files so I can easily verify that my quick test cases
> > match the real problems?
>
> Attached.

Thanks!

> > Based on the warning alone I think the test case below shows
> > the problem:
> >
> >   __thread int i __attribute__ ((tls_model ("local-exec")));
> >   extern __attribute__ ((alias ("i"), copy (i))) int j;
> >
> > tls_model is one of the few attributes I didn't test.  The warning
> > suggests the copy attribute code might need to filter out some more
> > attributes.  Let me work on that.
>
> Or maybe some variable of libc_hidden_data_def is needed that uses
> __thread when defining aliases, in which case tls_model attributes might
> not be considered to be ignored?  (The declarations in soft-supp.h use
> libc_hidden_tls_proto which does use __thread when defining aliases.)

Attached is a small test case reduced from the TU.

Attribute copy calls decl_attributes to copy tls_model from
__sim_exceptions_thread to __EI___sim_exceptions_thread.  That
triggers a warning because
DECL_THREAD_LOCAL_P (__EI___sim_exceptions_thread) is false.
I think __EI___sim_exceptions_thread needs to be declared
thread-local (just like __sim_exceptions_thread) for this
to work.  Can you think of something better?

> > Does it look close to what's going on in the file?  If so, does
> > it make sense to define an alias target inline/always_inline?
> > (I can filter attribute always_inline out of copy but I wonder
> > if changing the target to avoid always_inline would be more
> > appropriate.)
>
> I can imagine there might be uses for having some aliases that should be
> inlined and others that shouldn't be (provided there is actually an
> external definition for the function - if it's GNU extern inline with no
> separate definition provided, aliases aren't exactly useful).

Okay, let me just exclude always_inline and gnu_inline from
the attributes to copy and not worry about this as a potential
problem for now.

> > So it sounds like the mips16 attribute is meaningful on definitions
> > but doesn't impact callers, correct?  I think it would be useful to
>
> Strictly speaking I think it *can* impact callers by telling them whether
> they might need to generate code compatible with interlinking mips16 and
> non-mips16 for that call (in the absence of the attribute, the caller
> needs to make safe assumptions about not knowing what instruction set the
> function is built for).
>
> The purpose of the alias, however, is definitely to avoid problems with
> declarations of __longjmp being inconsistent as to whether they have the
> attribute; see
> <https://sourceware.org/ml/libc-ports/2013-01/msg00047.html>.

Hmm.  I confess I'm confused by this and by the referenced
discussions.  Most of it sounds like the alias in this case
suppresses a valid error: the mismatch between the caller and
the callee.  But then the conclusion seems to be that
declaring a function one way and defining it another is (or
can be, perhaps with some care) okay.  So either issuing
an error for the mismatch between declarations of the same
function is overly strict (in which case a warning for it
should be appropriate, as would be the attribute warning) or
the error does makes sense in which case so should the attribute
warning.

I'm open to filtering out these two attributes since it doesn't
seem that important one way or the other but unless I'm even
more confused than I think I am I would prefer to keep things
as they are and change Glibc to suppress the warning.  Partly
because of the rationale I just gave and in part also because
I don't want to hardcode a target attribute into c-attribs.c
and I'm not convinced yet we need to introduce some sort of
a hook for this (though ultimately we might end up going that
route).

Let me know if you see a problem with suppressing the warning
in Glibc.

Martin

Attachment: sim-full.c
Description: Text document