Re: [PATCH] add simple attribute introspection

[Martin Sebor <msebor at gmail dot com>]

On 11/22/18 5:28 AM, Christophe Lyon wrote:
> On Sat, 10 Nov 2018 at 00:43, Martin Sebor <msebor@gmail.com> wrote:
> > On 11/09/2018 12:59 PM, Jeff Law wrote:
> > > On 10/31/18 10:27 AM, Martin Sebor wrote:
> > > > Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
> > > >
> > > > With the C++ bits approved I'm still looking for a review/approval
> > > > of the remaining changes: the C front end and the shared c-family
> > > > handling of the new built-in.
> > > I thought I acked those with just a couple minor doc nits:
> >
> > I don't see a formal approval for the rest in my Inbox or in
> > the archive.
> >
> > > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> > > > index 8ffb0cd..dcf4747 100644
> > > > --- a/gcc/doc/extend.texi
> > > > +++ b/gcc/doc/extend.texi
> > > > @@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes
> > > are still necessary.
> > > >   @cindex @code{flatten} function attribute
> > > >   Generally, inlining into a function is limited.  For a function
> > > marked with
> > > >   this attribute, every call inside this function is inlined, if possible.
> > > > -Whether the function itself is considered for inlining depends on its
> > > size and
> > > > -the current inlining parameters.
> > > > +Functions declared with attribute @code{noinline} and similar are not
> > > > +inlined.  Whether the function itself is considered for inlining depends
> > > > +on its size and the current inlining parameters.
> > > Guessing this was from another doc patch that I think has already been
> > > approved
> >
> > Yes.  It shouldn't be in the latest patch at the link above.
> >
> > > > @@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
> > > >
> > > >   @end deftypefn
> > > >
> > > > +@deftypefn {Built-in Function} bool __builtin_has_attribute
> > > (@var{type-or-expression}, @var{attribute})
> > > > +The @code{__builtin_has_attribute} function evaluates to an integer
> > > constant
> > > > +expression equal to @code{true} if the symbol or type referenced by
> > > > +the @var{type-or-expression} argument has been declared with
> > > > +the @var{attribute} referenced by the second argument.  Neither argument
> > > > +is valuated.  The @var{type-or-expression} argument is subject to the
> > > same
> > > s/valuated/evaluated/ ?
> >
> > This should also be fixed in the latest patch at the link above.
> >
> > > Did the implementation change significantly requiring another review
> > > iteration?
> >
> > I don't think it changed too significantly between the last two
> > revisions but I don't have a record of anyone having approved
> > the C FE and the middle-end bits.  (Sorry if I missed it.) Other
> > than this response from you all I see in the archive is this:
> >
> >     https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00606.html
> >
> > Please let me if the last revision is okay to commit.
>
> Hi,
>
> It seems you committed this yesterday as r266335, and I have noticed
> new failures:
> on both aarch64 and arm:
> FAIL: c-c++-common/builtin-has-attribute-3.c  -Wc++-compat  (test for
> excess errors)
>
> gcc.log says:
> Excess errors:
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:11:25: error:
> alignment for 'faligned_1' must be at least 4

Test fails on targets whose default function alignment is 4 (or
any other value greater than 1) because the attribute handler
rejects attribute aligned with less restrictive arguments.

That seems unnecessary to me because less restrictive alignments
are trivially satisfied by more restrictive ones.  Clang accepts
the code and I'd say it makes sense for GCC to do the same.
The GNU assembler for aarch64 and sparc-solaris2.11 don't seem
to mind it (though it doesn't look like GCC actually emits .align
directives for small alignments; that may need a separate change
from one to relax the handler).

> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:12:25: error:
> alignment for 'faligned_2' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:39:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:40:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:47:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:48:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:50:3: error:
> size of array 'Assert' is negative
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
> size of array 'Assert' is negative
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:53:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:56:3: error:
> size of array 'Assert' is negative
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:58:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
> size of array 'Assert' is negative
>
> on arm only:
> gcc.dg/builtin-has-attribute.c (test for excess errors)
> gdb.log says:
> Excess errors:
> /gcc/testsuite/gcc.dg/builtin-has-attribute.c:12:15: error: size of
> array 'Assert' is negative

This failure is caused by the TYPE_USER_ALIGN bit getting cleared
for a type:

  struct __attribute__ ((aligned (8))) S { int i; }
  _Static_assert (__builtin_has_attribute (struct S, aligned));

The assertion passes with the aarch64 and x86_64 back crosses but
not with arm or sparc.  The bit is cleared in finalize_type_size:

static void
finalize_type_size (tree type)
{
  /* Normally, use the alignment corresponding to the mode chosen.
     However, where strict alignment is not required, avoid
     over-aligning structures, since most compilers do not do this
     alignment.  */
  if (TYPE_MODE (type) != BLKmode
      && TYPE_MODE (type) != VOIDmode
      && (STRICT_ALIGNMENT || !AGGREGATE_TYPE_P (type)))
    {
      unsigned mode_align = GET_MODE_ALIGNMENT (TYPE_MODE (type));

      /* Don't override a larger alignment requirement coming from a user
	 alignment of one of the fields.  */
      if (mode_align >= TYPE_ALIGN (type))
	{
	  SET_TYPE_ALIGN (type, mode_align);
	  TYPE_USER_ALIGN (type) = 0;
	}
    }

  /* Do machine-dependent extra alignment.  */
#ifdef ROUND_TYPE_ALIGN
  SET_TYPE_ALIGN (type,
                  ROUND_TYPE_ALIGN (type, TYPE_ALIGN (type), 
BITS_PER_UNIT));
#endif
  ...

has_attribute needs to handle this.

Martin