[PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

The __builtin_has_attribute function fails with an ICE when
its first argument is an expression such as INDIRECT_REF, or
many others.  The code simply assumes it's either a type or
a decl.  The attached patch corrects this oversight.

While testing the fix I also noticed the C++ front end expects
the first operand to be a unary expression, which causes most
other kinds of expressions to be rejected.  The patch fixes
that as well.

Finally, while testing the fix even more I realized that
the built-in considers the underlying array itself in ARRAY_REF
expressions rather than its type, which leads to inconsistent
results for overaligned arrays (it's the array itself that's
overaligned, not its elements).  So I fixed that too and
adjusted the one test designed to verify this.

Tested on x86_64-linux.

Martin

[-- Attachment #2: gcc-88383.diff --]
[-- Type: text/x-patch, Size: 7169 bytes --]

PR c/88383 - ICE calling __builtin_has_attribute on a reference

gcc/c-family/ChangeLog:

	PR c/88383
	* c-attribs.c (validate_attribute): Handle expressions.
	(has_attribute): Handle types referenced by expressions.
	Avoid considering array attributes in ARRAY_REF expressions .

gcc/cp/ChangeLog:

	PR c/88383
	* parser.c (cp_parser_has_attribute_expression): Handle assignment
	expressions.

gcc/testsuite/ChangeLog:

	PR c/88383
	* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
	* c-c++-common/builtin-has-attribute-6.c: New test.

Index: gcc/c-family/c-attribs.c
===================================================================
--- gcc/c-family/c-attribs.c	(revision 266799)
+++ gcc/c-family/c-attribs.c	(working copy)
@@ -3994,8 +3994,10 @@ validate_attribute (location_t atloc, tree oper, t
 
   if (TYPE_P (oper))
     tmpdecl = build_decl (atloc, TYPE_DECL, tmpid, oper);
-  else
+  else if (DECL_P (oper))
     tmpdecl = build_decl (atloc, TREE_CODE (oper), tmpid, TREE_TYPE (oper));
+  else if (EXPR_P (oper))
+    tmpdecl = build_decl (atloc, TYPE_DECL, tmpid, TREE_TYPE (oper));
 
   /* Temporarily clear CURRENT_FUNCTION_DECL to make decl_attributes
      believe the DECL declared above is at file scope.  (See bug 87526.)  */
@@ -4040,11 +4042,17 @@ has_attribute (location_t atloc, tree t, tree attr
       do
 	{
 	  /* Determine the array element/member declaration from
-	     an ARRAY/COMPONENT_REF.  */
+	     a COMPONENT_REF and an INDIRECT_REF involving a refeence.  */
 	  STRIP_NOPS (t);
 	  tree_code code = TREE_CODE (t);
-	  if (code == ARRAY_REF)
-	    t = TREE_OPERAND (t, 0);
+	  if (code == INDIRECT_REF)
+	    {
+	      tree op0 = TREE_OPERAND (t, 0);
+	      if (TREE_CODE (TREE_TYPE (op0)) == REFERENCE_TYPE)
+		t = op0;
+	      else
+		break;
+	    }
 	  else if (code == COMPONENT_REF)
 	    t = TREE_OPERAND (t, 1);
 	  else
@@ -4095,7 +4103,8 @@ has_attribute (location_t atloc, tree t, tree attr
 	}
       else
 	{
-	  atlist = TYPE_ATTRIBUTES (TREE_TYPE (expr));
+	  type = TREE_TYPE (expr);
+	  atlist = TYPE_ATTRIBUTES (type);
 	  done = true;
 	}
 
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 266799)
+++ gcc/cp/parser.c	(working copy)
@@ -8480,9 +8480,9 @@ cp_parser_has_attribute_expression (cp_parser *par
   cp_parser_parse_definitely (parser);
 
   /* If the type-id production did not work out, then we must be
-     looking at the unary-expression production.  */
+     looking at an expression.  */
   if (!oper || oper == error_mark_node)
-    oper = cp_parser_unary_expression (parser);
+    oper = cp_parser_assignment_expression (parser);
 
   /* Go back to evaluating expressions.  */
   --cp_unevaluated_operand;
Index: gcc/testsuite/c-c++-common/builtin-has-attribute-4.c
===================================================================
--- gcc/testsuite/c-c++-common/builtin-has-attribute-4.c	(revision 266799)
+++ gcc/testsuite/c-c++-common/builtin-has-attribute-4.c	(working copy)
@@ -151,7 +151,8 @@ void test_packed (struct PackedMember *p)
   A (0, gpak[0].c, packed);
   A (0, gpak[1].s, packed);
   A (1, gpak->a, packed);
-  A (1, (*gpak).a[0], packed);
+  /* It's the array that's declared packed but not its elements.  */
+  A (0, (*gpak).a[0], packed);
 
   /* The following fails because in C it's represented as
        INDIRECT_REF (POINTER_PLUS (NOP_EXPR (ADDR_EXPR (gpak)), ...))
@@ -161,7 +162,8 @@ void test_packed (struct PackedMember *p)
   A (0, p->c, packed);
   A (0, p->s, packed);
   A (1, p->a, packed);
-  A (1, p->a[0], packed);
+  /* It's the array that's declared packed but not its elements.  */
+  A (0, p->a[0], packed);
   /* Similar to the comment above.
    A (1, *p->a, packed);  */
 }
Index: gcc/testsuite/c-c++-common/builtin-has-attribute-6.c
===================================================================
--- gcc/testsuite/c-c++-common/builtin-has-attribute-6.c	(nonexistent)
+++ gcc/testsuite/c-c++-common/builtin-has-attribute-6.c	(working copy)
@@ -0,0 +1,105 @@
+/* PR c/88383 - ICE calling _builtin_has_attribute(r, aligned(N)))
+   on an overaligned reference r
+   { dg-options "-Wall -ftrack-macro-expansion=0" }
+   { dg-options "-Wall -Wno-narrowing -Wno-unused -ftrack-macro-expansion=0" { target c++ } }  */
+
+#define ATTR(...) __attribute__ ((__VA_ARGS__))
+
+#define A(expect, sym, attr)						\
+  typedef int Assert [1 - 2 * !(__builtin_has_attribute (sym, attr) == expect)]
+
+typedef ATTR (aligned (8)) int Int8;
+
+/* The attribute applies to the array, not to the type of its elements.  */
+extern ATTR (aligned (8)) char i8arr[];
+
+/* The attribute applies to the pointer, not to the type it points to.  */
+extern ATTR (aligned (8)) int *ptr;
+extern Int8 *i8ptr;
+
+#if __cplusplus
+
+/* Similarly here, the attribute applies to the reference, not to its type.  */
+extern ATTR (aligned (8)) int &ref;
+extern Int8 &i8ref;
+
+#else
+
+/* Fake references in C.  */
+extern ATTR (aligned (8)) int ref;
+Int8 i8ref;
+
+#endif
+
+void test (void)
+{
+  /* Verify that the built-in detects the attribute on the array. */
+  A (1, i8arr, aligned);
+  A (0, i8arr, aligned (1));
+  A (0, i8arr, aligned (2));
+  A (0, i8arr, aligned (4));
+  A (1, i8arr, aligned (8));
+  A (0, i8arr, aligned (16));
+
+  A (0, i8arr + 1, aligned);
+  A (0, i8arr + 2, aligned (1));
+  A (0, i8arr + 3, aligned (8));
+
+  /* Verify the builtin detects the absence of the attribute on
+     the elements.  */
+  A (0, i8arr[0], aligned);
+  A (0, *i8arr,   aligned);
+
+  /* Verify that the built-in detects the attribute on the pointer
+     itself. */
+  A (1, ptr, aligned);
+  A (0, ptr, aligned (1));
+  A (0, ptr, aligned (2));
+  A (0, ptr, aligned (4));
+  A (1, ptr, aligned (8));
+  A (0, ptr, aligned (16));
+
+  A (0, ptr + 1, aligned);
+  A (0, ptr + 2, aligned (1));
+  A (0, ptr + 3, aligned (8));
+
+  /* The pointed to type is not declared with attribute aligned.  */
+  A (0, *ptr, aligned);
+  A (0, *ptr, aligned (1));
+  A (0, *ptr, aligned (2));
+  A (0, *ptr, aligned (4));
+  A (0, *ptr, aligned (8));
+  A (0, *ptr, aligned (16));
+
+  A (0, *ptr + 1, aligned);
+  A (0, *ptr + 2, aligned (1));
+  A (0, *ptr + 3, aligned (8));
+
+  /* Verify that the built-in correctly detects the attribute on
+     the type of the lvalue referenced by the pointer. */
+  A (0, i8ptr,     aligned);
+  A (0, i8ptr,     aligned (8));
+  A (0, i8ptr + 1, aligned);
+  A (0, i8ptr + 3, aligned (8));
+  A (1, *i8ptr,    aligned);
+  A (0, *i8ptr,    aligned (1));
+  A (0, *i8ptr,    aligned (2));
+  A (0, *i8ptr,    aligned (4));
+  A (1, *i8ptr,    aligned (8));
+  A (0, *i8ptr,    aligned (16));
+
+  /* The reference itself is declared aligned, even though the type
+     it refers to isn't.  But see PR c++/88362.  */
+  A (1, ref, aligned);
+  A (0, ref, aligned (1));
+  A (0, ref, aligned (2));
+  A (0, ref, aligned (4));
+  A (1, ref, aligned (8));
+  A (0, ref, aligned (16));
+
+  /* Also verify that assignment expressions are accepted.  */
+  A (0, ref = 1,  aligned);
+  A (0, ref += 2, aligned (1));
+  A (0, ref /= 3, aligned (8));
+
+}

Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Jeff Law]

On 12/5/18 8:55 PM, Martin Sebor wrote:
> The __builtin_has_attribute function fails with an ICE when
> its first argument is an expression such as INDIRECT_REF, or
> many others.  The code simply assumes it's either a type or
> a decl.  The attached patch corrects this oversight.
> 
> While testing the fix I also noticed the C++ front end expects
> the first operand to be a unary expression, which causes most
> other kinds of expressions to be rejected.  The patch fixes
> that as well.
> 
> Finally, while testing the fix even more I realized that
> the built-in considers the underlying array itself in ARRAY_REF
> expressions rather than its type, which leads to inconsistent
> results for overaligned arrays (it's the array itself that's
> overaligned, not its elements).  So I fixed that too and
> adjusted the one test designed to verify this.
> 
> Tested on x86_64-linux.
> 
> Martin
> 
> gcc-88383.diff
> 
> PR c/88383 - ICE calling __builtin_has_attribute on a reference
> 
> gcc/c-family/ChangeLog:
> 
> 	PR c/88383
> 	* c-attribs.c (validate_attribute): Handle expressions.
> 	(has_attribute): Handle types referenced by expressions.
> 	Avoid considering array attributes in ARRAY_REF expressions .
> 
> gcc/cp/ChangeLog:
> 
> 	PR c/88383
> 	* parser.c (cp_parser_has_attribute_expression): Handle assignment
> 	expressions.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c/88383
> 	* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
> 	* c-c++-common/builtin-has-attribute-6.c: New test.
Well, the high level question here is do we want to support this builtin
on expressions at all.  Generally attributes apply to types and decls,
not expressions.

Clearly we shouldn't fault, but my first inclination is that the
attribute applies to types or decls, not expressions.  In that case we
should just be issuing an error.

I could be convinced otherwise, so if you think we should support
passing expressions to this builtin, make your case.

jeff

Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Martin Sebor]

On 12/13/18 11:59 AM, Jeff Law wrote:
> On 12/5/18 8:55 PM, Martin Sebor wrote:
>> The __builtin_has_attribute function fails with an ICE when
>> its first argument is an expression such as INDIRECT_REF, or
>> many others.  The code simply assumes it's either a type or
>> a decl.  The attached patch corrects this oversight.
>>
>> While testing the fix I also noticed the C++ front end expects
>> the first operand to be a unary expression, which causes most
>> other kinds of expressions to be rejected.  The patch fixes
>> that as well.
>>
>> Finally, while testing the fix even more I realized that
>> the built-in considers the underlying array itself in ARRAY_REF
>> expressions rather than its type, which leads to inconsistent
>> results for overaligned arrays (it's the array itself that's
>> overaligned, not its elements).  So I fixed that too and
>> adjusted the one test designed to verify this.
>>
>> Tested on x86_64-linux.
>>
>> Martin
>>
>> gcc-88383.diff
>>
>> PR c/88383 - ICE calling __builtin_has_attribute on a reference
>>
>> gcc/c-family/ChangeLog:
>>
>> 	PR c/88383
>> 	* c-attribs.c (validate_attribute): Handle expressions.
>> 	(has_attribute): Handle types referenced by expressions.
>> 	Avoid considering array attributes in ARRAY_REF expressions .
>>
>> gcc/cp/ChangeLog:
>>
>> 	PR c/88383
>> 	* parser.c (cp_parser_has_attribute_expression): Handle assignment
>> 	expressions.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	PR c/88383
>> 	* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
>> 	* c-c++-common/builtin-has-attribute-6.c: New test.
> Well, the high level question here is do we want to support this builtin
> on expressions at all.  Generally attributes apply to types and decls,
> not expressions.
> 
> Clearly we shouldn't fault, but my first inclination is that the
> attribute applies to types or decls, not expressions.  In that case we
> should just be issuing an error.
> 
> I could be convinced otherwise, so if you think we should support
> passing expressions to this builtin, make your case.

The support is necessary in order to determine the attributes
in expressions such as:

   struct S { __attribute__ ((packed)) int a[32]; };

   extern struct S s;

   _Static_assert (__builtin_has_attribute (s.a, packed));

Martin

Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Martin Sebor]

On 12/13/18 12:20 PM, Martin Sebor wrote:
> On 12/13/18 11:59 AM, Jeff Law wrote:
>> On 12/5/18 8:55 PM, Martin Sebor wrote:
>>> The __builtin_has_attribute function fails with an ICE when
>>> its first argument is an expression such as INDIRECT_REF, or
>>> many others.  The code simply assumes it's either a type or
>>> a decl.  The attached patch corrects this oversight.
>>>
>>> While testing the fix I also noticed the C++ front end expects
>>> the first operand to be a unary expression, which causes most
>>> other kinds of expressions to be rejected.  The patch fixes
>>> that as well.
>>>
>>> Finally, while testing the fix even more I realized that
>>> the built-in considers the underlying array itself in ARRAY_REF
>>> expressions rather than its type, which leads to inconsistent
>>> results for overaligned arrays (it's the array itself that's
>>> overaligned, not its elements).  So I fixed that too and
>>> adjusted the one test designed to verify this.
>>>
>>> Tested on x86_64-linux.
>>>
>>> Martin
>>>
>>> gcc-88383.diff
>>>
>>> PR c/88383 - ICE calling __builtin_has_attribute on a reference
>>>
>>> gcc/c-family/ChangeLog:
>>>
>>>     PR c/88383
>>>     * c-attribs.c (validate_attribute): Handle expressions.
>>>     (has_attribute): Handle types referenced by expressions.
>>>     Avoid considering array attributes in ARRAY_REF expressions .
>>>
>>> gcc/cp/ChangeLog:
>>>
>>>     PR c/88383
>>>     * parser.c (cp_parser_has_attribute_expression): Handle assignment
>>>     expressions.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>     PR c/88383
>>>     * c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
>>>     * c-c++-common/builtin-has-attribute-6.c: New test.
>> Well, the high level question here is do we want to support this builtin
>> on expressions at all.  Generally attributes apply to types and decls,
>> not expressions.
>>
>> Clearly we shouldn't fault, but my first inclination is that the
>> attribute applies to types or decls, not expressions.  In that case we
>> should just be issuing an error.
>>
>> I could be convinced otherwise, so if you think we should support
>> passing expressions to this builtin, make your case.
> 
> The support is necessary in order to determine the attributes
> in expressions such as:
> 
>    struct S { __attribute__ ((packed)) int a[32]; };
> 
>    extern struct S s;
> 
>    _Static_assert (__builtin_has_attribute (s.a, packed));

An example involving types might be a better one:

   typedef __attribute__ ((may_alias)) int* BadInt;

   void f (BadInt *p)
   {
     _Static_assert (__builtin_has_attribute (*p, may_alias));
   }

Martin

Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Jakub Jelinek]

On Thu, Dec 13, 2018 at 12:48:18PM -0700, Martin Sebor wrote:
> > The support is necessary in order to determine the attributes
> > in expressions such as:
> > 
> >    struct S { __attribute__ ((packed)) int a[32]; };
> > 
> >    extern struct S s;
> > 
> >    _Static_assert (__builtin_has_attribute (s.a, packed));
> 
> An example involving types might be a better one:
> 
>   typedef __attribute__ ((may_alias)) int* BadInt;
> 
>   void f (BadInt *p)
>   {
>     _Static_assert (__builtin_has_attribute (*p, may_alias));
>   }

So how is the builtin defined then?  Is the argument always an expression
and you only return whether its type has the attribute, or do something
different if the expression is of certain kind?
Perhaps it would be better have two builtins, one would always take
type from the expression, the other would only accept decls (or perhaps
some syntax for the FIELD_DECLs).

	Jakub

Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Martin Sebor]

On 12/13/18 12:56 PM, Jakub Jelinek wrote:
> On Thu, Dec 13, 2018 at 12:48:18PM -0700, Martin Sebor wrote:
>>> The support is necessary in order to determine the attributes
>>> in expressions such as:
>>>
>>>     struct S { __attribute__ ((packed)) int a[32]; };
>>>
>>>     extern struct S s;
>>>
>>>     _Static_assert (__builtin_has_attribute (s.a, packed));
>>
>> An example involving types might be a better one:
>>
>>    typedef __attribute__ ((may_alias)) int* BadInt;
>>
>>    void f (BadInt *p)
>>    {
>>      _Static_assert (__builtin_has_attribute (*p, may_alias));
>>    }
> 
> So how is the builtin defined then?  Is the argument always an expression
> and you only return whether its type has the attribute, or do something
> different if the expression is of certain kind?

For a DECL the built-in looks at both the DECL_ATTRIBUTES and
the DECL's type's TYPE_ATTRIBUTES.  For expressions, it just
looks at TYPE_ATTRIBUTES of the expression's type.  I'm not
sure that this is necessarily the cleanest design but it's
meant to follow how attributes are applied to DECLs.  Sometimes
they get applied to the DECL itself and other times to its type.
This has been causing confusion to both users and GCC devs such
as myself so at some point I'd like to make this clearer somehow.
Not sure exactly how yet.

> Perhaps it would be better have two builtins, one would always take
> type from the expression, the other would only accept decls (or perhaps
> some syntax for the FIELD_DECLs).

I'm not sure I see a need for two built-ins here.

There is another way to handle the may_alias example: by using
typeof first and then the built-in on the result:

   _Static_assert (__builtin_has_attribute (__typeof__ (*p), may_alias));

so it seems that it would be possible to do what Jeff suggests
and only accept DECLs and TYPEs.  But I'm also not sure I see
an advantage of restricting the built-in like that.

FWIW, I had in mind two uses when I introduced the built-in:
1) a conditional attribute copy (where the condition would be
whether or not the source DECL or TYPE has some attribute), and
2) detecting that attributes have been successfully applied as
expected in tests.  I haven't implemented (1) yet but I'd like
to at some point (if only to avoid hardcoding into GCC the set
of attributes that are excluded from copying).  I have added
a handful of tests that make use of the built-in.  So if changes
should be made to the built-in, I will want to make sure they
don't make these use cases difficult.

Martin

Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Jakub Jelinek]

On Thu, Dec 13, 2018 at 02:05:29PM -0700, Martin Sebor wrote:
> > So how is the builtin defined then?  Is the argument always an expression
> > and you only return whether its type has the attribute, or do something
> > different if the expression is of certain kind?
> 
> For a DECL the built-in looks at both the DECL_ATTRIBUTES and
> the DECL's type's TYPE_ATTRIBUTES.  For expressions, it just
> looks at TYPE_ATTRIBUTES of the expression's type.  I'm not
> sure that this is necessarily the cleanest design but it's
> meant to follow how attributes are applied to DECLs.  Sometimes
> they get applied to the DECL itself and other times to its type.
> This has been causing confusion to both users and GCC devs such
> as myself so at some point I'd like to make this clearer somehow.
> Not sure exactly how yet.

But some users of the attribute look for the attribute on TYPE_ATTRIBUTES,
other on DECL_ATTRIBUTES, and the attributes have bools which say to what
they apply.  So, I think the API need to be very clear whether the attribute
is on a decl or on a type.

If it is similar to say sizeof/__alignof__ etc., where it accepts either
a type (for which one can use __typeof/decltype etc.), in which case it
would check the type attributes, or an expression and in that case require
it to be a decl and use decl attributes, one API is fine, but supporting
arbitrary expressions and sometimes looking at type, other times at decl
or both is not a good design.

	Jakub

Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Martin Sebor]

On 12/13/18 4:39 PM, Jakub Jelinek wrote:
> On Thu, Dec 13, 2018 at 02:05:29PM -0700, Martin Sebor wrote:
>>> So how is the builtin defined then?  Is the argument always an expression
>>> and you only return whether its type has the attribute, or do something
>>> different if the expression is of certain kind?
>>
>> For a DECL the built-in looks at both the DECL_ATTRIBUTES and
>> the DECL's type's TYPE_ATTRIBUTES.  For expressions, it just
>> looks at TYPE_ATTRIBUTES of the expression's type.  I'm not
>> sure that this is necessarily the cleanest design but it's
>> meant to follow how attributes are applied to DECLs.  Sometimes
>> they get applied to the DECL itself and other times to its type.
>> This has been causing confusion to both users and GCC devs such
>> as myself so at some point I'd like to make this clearer somehow.
>> Not sure exactly how yet.
> 
> But some users of the attribute look for the attribute on TYPE_ATTRIBUTES,
> other on DECL_ATTRIBUTES, and the attributes have bools which say to what
> they apply.  So, I think the API need to be very clear whether the attribute
> is on a decl or on a type.
> 
> If it is similar to say sizeof/__alignof__ etc., where it accepts either
> a type (for which one can use __typeof/decltype etc.), in which case it
> would check the type attributes, or an expression and in that case require
> it to be a decl and use decl attributes, one API is fine, but supporting
> arbitrary expressions and sometimes looking at type, other times at decl
> or both is not a good design.

It's as good as the design of GCC attributes allows.  Based on
the manual a function declaration like

   __attribute__ ((alloc_size (1), malloc))
   void* allocate (unsigned);

should have those two attributes applied to it.  Yet, alloc_size
is actually applied to its type (but not to its decl) while malloc
to the function's decl but not its type (bug 88397).

I'm pretty sure most users still expect the following to pass:

   _Static_assert (__builtin_has_attribute (allocate, alloc_size));
   _Static_assert (__builtin_has_attribute (allocate, malloc));

It wouldn't if the built-in differentiated between decls and types.

Even if/when we make these function attributes (and others like it)
apply to types as I think we should so it's possible to apply them
to function pointers as users expect, there still will be others
that will apply only to the function decls.  The distinction
between when GCC chooses to apply an attribute to a function decl
vs to its type is not intuitive and cannot very well be, and neither
would be an API that queried just a decl attribute or just a type
attribute.  The most common use case is to query whether a function
has been declared with an attribute -- it doesn't matter whether
it's on its unique type or on its decl.  (The others might perhaps
be useful too, but a lot less often.)

Martin

Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Jakub Jelinek]

On Thu, Dec 13, 2018 at 09:03:57PM -0700, Martin Sebor wrote:
> It's as good as the design of GCC attributes allows.  Based on

No.

> the manual a function declaration like
> 
>   __attribute__ ((alloc_size (1), malloc))
>   void* allocate (unsigned);
> 
> should have those two attributes applied to it.  Yet, alloc_size
> is actually applied to its type (but not to its decl) while malloc
> to the function's decl but not its type (bug 88397).
> 
> I'm pretty sure most users still expect the following to pass:
> 
>   _Static_assert (__builtin_has_attribute (allocate, alloc_size));
>   _Static_assert (__builtin_has_attribute (allocate, malloc));

Users shouldn't expect this.  If anything, we should document what
attributes are type attributes and which attributes are declaration
attributes.

With the way you're proposing, users could check the type attributes
simply through __typeof/decltype etc., but couldn't test solely the
declaration attributes.

	Jakub

Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Martin Sebor]

>> the manual a function declaration like
>>
>>    __attribute__ ((alloc_size (1), malloc))
>>    void* allocate (unsigned);
>>
>> should have those two attributes applied to it.  Yet, alloc_size
>> is actually applied to its type (but not to its decl) while malloc
>> to the function's decl but not its type (bug 88397).
>>
>> I'm pretty sure most users still expect the following to pass:
>>
>>    _Static_assert (__builtin_has_attribute (allocate, alloc_size));
>>    _Static_assert (__builtin_has_attribute (allocate, malloc));
> 
> Users shouldn't expect this.  If anything, we should document what
> attributes are type attributes and which attributes are declaration
> attributes.

I designed the built-in based on what I expect.

The programs that care about whether a function is declared,
say with attribute alloc_align or malloc, do not and should
not have to worry about whether the attribute is on the decl
or on its type -- in the expected use cases it makes no
difference.  Those that might care whether an attribute is
on the type can easily check the type:

   __builtin_has_attribute (__typeof__ (allocate), alloc_size)

(I would expect GCC to apply an attribute either to a decl or
to a type but not to both.)

I do agree that whether an attribute applies to a function or
its type should be reviewed and where it makes sense documented.
More than that, some attributes that currently only apply to
function decls should be changed to apply to (function) types
instead so that calls via pointers to such functions can get
the same benefits as calls to the functions themselves.  Malloc
is an example (bug 88397).

> With the way you're proposing, users could check the type attributes
> simply through __typeof/decltype etc., but couldn't test solely the
> declaration attributes.

I'm not proposing anything -- what I described is the design.
I don't know of a use case for testing the decl alone for
attributes.  In all those I can think of, what matters is
the union of attributes between the decl and its type.

But I'm not opposed to enhancing the function if an important
use case does turn up that's not supported.  For what you are
asking for this should already do it so I don't see a need to
change anything:

   #define decl_has_attribute(d, ...)  \
      (__builtin_has_attribute (d, __VA_ARGS__)  \
       && !__builtin_has_attribute (__typeof__ (d), __VA_ARGS__))

Martin

Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Martin Sebor]

Jeff, did this and the rest of the discussion answer your question
and if so, is the patch okay to commit?

   https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html

Martin

On 12/13/18 12:48 PM, Martin Sebor wrote:
> On 12/13/18 12:20 PM, Martin Sebor wrote:
>> On 12/13/18 11:59 AM, Jeff Law wrote:
>>> On 12/5/18 8:55 PM, Martin Sebor wrote:
>>>> The __builtin_has_attribute function fails with an ICE when
>>>> its first argument is an expression such as INDIRECT_REF, or
>>>> many others.  The code simply assumes it's either a type or
>>>> a decl.  The attached patch corrects this oversight.
>>>>
>>>> While testing the fix I also noticed the C++ front end expects
>>>> the first operand to be a unary expression, which causes most
>>>> other kinds of expressions to be rejected.  The patch fixes
>>>> that as well.
>>>>
>>>> Finally, while testing the fix even more I realized that
>>>> the built-in considers the underlying array itself in ARRAY_REF
>>>> expressions rather than its type, which leads to inconsistent
>>>> results for overaligned arrays (it's the array itself that's
>>>> overaligned, not its elements).  So I fixed that too and
>>>> adjusted the one test designed to verify this.
>>>>
>>>> Tested on x86_64-linux.
>>>>
>>>> Martin
>>>>
>>>> gcc-88383.diff
>>>>
>>>> PR c/88383 - ICE calling __builtin_has_attribute on a reference
>>>>
>>>> gcc/c-family/ChangeLog:
>>>>
>>>>     PR c/88383
>>>>     * c-attribs.c (validate_attribute): Handle expressions.
>>>>     (has_attribute): Handle types referenced by expressions.
>>>>     Avoid considering array attributes in ARRAY_REF expressions .
>>>>
>>>> gcc/cp/ChangeLog:
>>>>
>>>>     PR c/88383
>>>>     * parser.c (cp_parser_has_attribute_expression): Handle assignment
>>>>     expressions.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>>     PR c/88383
>>>>     * c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
>>>>     * c-c++-common/builtin-has-attribute-6.c: New test.
>>> Well, the high level question here is do we want to support this builtin
>>> on expressions at all.  Generally attributes apply to types and decls,
>>> not expressions.
>>>
>>> Clearly we shouldn't fault, but my first inclination is that the
>>> attribute applies to types or decls, not expressions.  In that case we
>>> should just be issuing an error.
>>>
>>> I could be convinced otherwise, so if you think we should support
>>> passing expressions to this builtin, make your case.
>>
>> The support is necessary in order to determine the attributes
>> in expressions such as:
>>
>>    struct S { __attribute__ ((packed)) int a[32]; };
>>
>>    extern struct S s;
>>
>>    _Static_assert (__builtin_has_attribute (s.a, packed));
> 
> An example involving types might be a better one:
> 
>    typedef __attribute__ ((may_alias)) int* BadInt;
> 
>    void f (BadInt *p)
>    {
>      _Static_assert (__builtin_has_attribute (*p, may_alias));
>    }
> 
> Martin

PING [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Martin Sebor]

Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html

On 12/20/18 7:51 PM, Martin Sebor wrote:
> Jeff, did this and the rest of the discussion answer your question
> and if so, is the patch okay to commit?
> 
>    https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
> 
> Martin
> 
> On 12/13/18 12:48 PM, Martin Sebor wrote:
>> On 12/13/18 12:20 PM, Martin Sebor wrote:
>>> On 12/13/18 11:59 AM, Jeff Law wrote:
>>>> On 12/5/18 8:55 PM, Martin Sebor wrote:
>>>>> The __builtin_has_attribute function fails with an ICE when
>>>>> its first argument is an expression such as INDIRECT_REF, or
>>>>> many others.  The code simply assumes it's either a type or
>>>>> a decl.  The attached patch corrects this oversight.
>>>>>
>>>>> While testing the fix I also noticed the C++ front end expects
>>>>> the first operand to be a unary expression, which causes most
>>>>> other kinds of expressions to be rejected.  The patch fixes
>>>>> that as well.
>>>>>
>>>>> Finally, while testing the fix even more I realized that
>>>>> the built-in considers the underlying array itself in ARRAY_REF
>>>>> expressions rather than its type, which leads to inconsistent
>>>>> results for overaligned arrays (it's the array itself that's
>>>>> overaligned, not its elements).  So I fixed that too and
>>>>> adjusted the one test designed to verify this.
>>>>>
>>>>> Tested on x86_64-linux.
>>>>>
>>>>> Martin
>>>>>
>>>>> gcc-88383.diff
>>>>>
>>>>> PR c/88383 - ICE calling __builtin_has_attribute on a reference
>>>>>
>>>>> gcc/c-family/ChangeLog:
>>>>>
>>>>>     PR c/88383
>>>>>     * c-attribs.c (validate_attribute): Handle expressions.
>>>>>     (has_attribute): Handle types referenced by expressions.
>>>>>     Avoid considering array attributes in ARRAY_REF expressions .
>>>>>
>>>>> gcc/cp/ChangeLog:
>>>>>
>>>>>     PR c/88383
>>>>>     * parser.c (cp_parser_has_attribute_expression): Handle assignment
>>>>>     expressions.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>>     PR c/88383
>>>>>     * c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
>>>>>     * c-c++-common/builtin-has-attribute-6.c: New test.
>>>> Well, the high level question here is do we want to support this 
>>>> builtin
>>>> on expressions at all.  Generally attributes apply to types and decls,
>>>> not expressions.
>>>>
>>>> Clearly we shouldn't fault, but my first inclination is that the
>>>> attribute applies to types or decls, not expressions.  In that case we
>>>> should just be issuing an error.
>>>>
>>>> I could be convinced otherwise, so if you think we should support
>>>> passing expressions to this builtin, make your case.
>>>
>>> The support is necessary in order to determine the attributes
>>> in expressions such as:
>>>
>>>    struct S { __attribute__ ((packed)) int a[32]; };
>>>
>>>    extern struct S s;
>>>
>>>    _Static_assert (__builtin_has_attribute (s.a, packed));
>>
>> An example involving types might be a better one:
>>
>>    typedef __attribute__ ((may_alias)) int* BadInt;
>>
>>    void f (BadInt *p)
>>    {
>>      _Static_assert (__builtin_has_attribute (*p, may_alias));
>>    }
>>
>> Martin
> 

PING #2 [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Martin Sebor]

Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html

On 1/3/19 3:22 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
> 
> On 12/20/18 7:51 PM, Martin Sebor wrote:
>> Jeff, did this and the rest of the discussion answer your question
>> and if so, is the patch okay to commit?
>>
>>    https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
>>
>> Martin
>>
>> On 12/13/18 12:48 PM, Martin Sebor wrote:
>>> On 12/13/18 12:20 PM, Martin Sebor wrote:
>>>> On 12/13/18 11:59 AM, Jeff Law wrote:
>>>>> On 12/5/18 8:55 PM, Martin Sebor wrote:
>>>>>> The __builtin_has_attribute function fails with an ICE when
>>>>>> its first argument is an expression such as INDIRECT_REF, or
>>>>>> many others.  The code simply assumes it's either a type or
>>>>>> a decl.  The attached patch corrects this oversight.
>>>>>>
>>>>>> While testing the fix I also noticed the C++ front end expects
>>>>>> the first operand to be a unary expression, which causes most
>>>>>> other kinds of expressions to be rejected.  The patch fixes
>>>>>> that as well.
>>>>>>
>>>>>> Finally, while testing the fix even more I realized that
>>>>>> the built-in considers the underlying array itself in ARRAY_REF
>>>>>> expressions rather than its type, which leads to inconsistent
>>>>>> results for overaligned arrays (it's the array itself that's
>>>>>> overaligned, not its elements).  So I fixed that too and
>>>>>> adjusted the one test designed to verify this.
>>>>>>
>>>>>> Tested on x86_64-linux.
>>>>>>
>>>>>> Martin
>>>>>>
>>>>>> gcc-88383.diff
>>>>>>
>>>>>> PR c/88383 - ICE calling __builtin_has_attribute on a reference
>>>>>>
>>>>>> gcc/c-family/ChangeLog:
>>>>>>
>>>>>>     PR c/88383
>>>>>>     * c-attribs.c (validate_attribute): Handle expressions.
>>>>>>     (has_attribute): Handle types referenced by expressions.
>>>>>>     Avoid considering array attributes in ARRAY_REF expressions .
>>>>>>
>>>>>> gcc/cp/ChangeLog:
>>>>>>
>>>>>>     PR c/88383
>>>>>>     * parser.c (cp_parser_has_attribute_expression): Handle 
>>>>>> assignment
>>>>>>     expressions.
>>>>>>
>>>>>> gcc/testsuite/ChangeLog:
>>>>>>
>>>>>>     PR c/88383
>>>>>>     * c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
>>>>>>     * c-c++-common/builtin-has-attribute-6.c: New test.
>>>>> Well, the high level question here is do we want to support this 
>>>>> builtin
>>>>> on expressions at all.  Generally attributes apply to types and decls,
>>>>> not expressions.
>>>>>
>>>>> Clearly we shouldn't fault, but my first inclination is that the
>>>>> attribute applies to types or decls, not expressions.  In that case we
>>>>> should just be issuing an error.
>>>>>
>>>>> I could be convinced otherwise, so if you think we should support
>>>>> passing expressions to this builtin, make your case.
>>>>
>>>> The support is necessary in order to determine the attributes
>>>> in expressions such as:
>>>>
>>>>    struct S { __attribute__ ((packed)) int a[32]; };
>>>>
>>>>    extern struct S s;
>>>>
>>>>    _Static_assert (__builtin_has_attribute (s.a, packed));
>>>
>>> An example involving types might be a better one:
>>>
>>>    typedef __attribute__ ((may_alias)) int* BadInt;
>>>
>>>    void f (BadInt *p)
>>>    {
>>>      _Static_assert (__builtin_has_attribute (*p, may_alias));
>>>    }
>>>
>>> Martin
>>
> 

Re: PING #2 [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Martin Sebor]

Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html

Jeff, do you have any more questions/concerns or is this patch
good to commit?

Martin

On 1/7/19 5:32 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
> 
> On 1/3/19 3:22 PM, Martin Sebor wrote:
>> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
>>
>> On 12/20/18 7:51 PM, Martin Sebor wrote:
>>> Jeff, did this and the rest of the discussion answer your question
>>> and if so, is the patch okay to commit?
>>>
>>>    https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
>>>
>>> Martin
>>>
>>> On 12/13/18 12:48 PM, Martin Sebor wrote:
>>>> On 12/13/18 12:20 PM, Martin Sebor wrote:
>>>>> On 12/13/18 11:59 AM, Jeff Law wrote:
>>>>>> On 12/5/18 8:55 PM, Martin Sebor wrote:
>>>>>>> The __builtin_has_attribute function fails with an ICE when
>>>>>>> its first argument is an expression such as INDIRECT_REF, or
>>>>>>> many others.  The code simply assumes it's either a type or
>>>>>>> a decl.  The attached patch corrects this oversight.
>>>>>>>
>>>>>>> While testing the fix I also noticed the C++ front end expects
>>>>>>> the first operand to be a unary expression, which causes most
>>>>>>> other kinds of expressions to be rejected.  The patch fixes
>>>>>>> that as well.
>>>>>>>
>>>>>>> Finally, while testing the fix even more I realized that
>>>>>>> the built-in considers the underlying array itself in ARRAY_REF
>>>>>>> expressions rather than its type, which leads to inconsistent
>>>>>>> results for overaligned arrays (it's the array itself that's
>>>>>>> overaligned, not its elements).  So I fixed that too and
>>>>>>> adjusted the one test designed to verify this.
>>>>>>>
>>>>>>> Tested on x86_64-linux.
>>>>>>>
>>>>>>> Martin
>>>>>>>
>>>>>>> gcc-88383.diff
>>>>>>>
>>>>>>> PR c/88383 - ICE calling __builtin_has_attribute on a reference
>>>>>>>
>>>>>>> gcc/c-family/ChangeLog:
>>>>>>>
>>>>>>>     PR c/88383
>>>>>>>     * c-attribs.c (validate_attribute): Handle expressions.
>>>>>>>     (has_attribute): Handle types referenced by expressions.
>>>>>>>     Avoid considering array attributes in ARRAY_REF expressions .
>>>>>>>
>>>>>>> gcc/cp/ChangeLog:
>>>>>>>
>>>>>>>     PR c/88383
>>>>>>>     * parser.c (cp_parser_has_attribute_expression): Handle 
>>>>>>> assignment
>>>>>>>     expressions.
>>>>>>>
>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>
>>>>>>>     PR c/88383
>>>>>>>     * c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
>>>>>>>     * c-c++-common/builtin-has-attribute-6.c: New test.
>>>>>> Well, the high level question here is do we want to support this 
>>>>>> builtin
>>>>>> on expressions at all.  Generally attributes apply to types and 
>>>>>> decls,
>>>>>> not expressions.
>>>>>>
>>>>>> Clearly we shouldn't fault, but my first inclination is that the
>>>>>> attribute applies to types or decls, not expressions.  In that 
>>>>>> case we
>>>>>> should just be issuing an error.
>>>>>>
>>>>>> I could be convinced otherwise, so if you think we should support
>>>>>> passing expressions to this builtin, make your case.
>>>>>
>>>>> The support is necessary in order to determine the attributes
>>>>> in expressions such as:
>>>>>
>>>>>    struct S { __attribute__ ((packed)) int a[32]; };
>>>>>
>>>>>    extern struct S s;
>>>>>
>>>>>    _Static_assert (__builtin_has_attribute (s.a, packed));
>>>>
>>>> An example involving types might be a better one:
>>>>
>>>>    typedef __attribute__ ((may_alias)) int* BadInt;
>>>>
>>>>    void f (BadInt *p)
>>>>    {
>>>>      _Static_assert (__builtin_has_attribute (*p, may_alias));
>>>>    }
>>>>
>>>> Martin
>>>
>>
> 

Re: PING #2 [PATCH] handle expressions in __builtin_has_attribute (PR 88383)

[Jeff Law]

On 1/15/19 9:31 AM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
> 
> Jeff, do you have any more questions/concerns or is this patch
> good to commit?
I think both Jakub and I were concerned about handling expressions in
this code.  I don't recall a resolution on that fundamental question.

jeff