[PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

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

This is a repost of a patch for PR 88383 updated to also fix the just
reported PR 89288 (the original patch only partially handles this case).
The review of the first patch was derailed by questions about the design
of the built-in so the fix for the ICE was never approved.  I think
the ICEs should be fixed for GCC 9 and any open design questions should
be dealt with independently.

Martin

The patch for PR 88383 was originally posted last December:
   https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html

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

PR c/88383 - ICE calling __builtin_has_attribute on a reference
PR c/89288 - ICE in tree_code_size, at tree.c:865

gcc/c-family/ChangeLog:

	PR c/88383
	PR c/89288
	* 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
	PR c/89288
	* parser.c (cp_parser_has_attribute_expression): Handle assignment
	expressions.

gcc/testsuite/ChangeLog:

	PR c/88383
	PR c/89288
	* 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 268774)
+++ gcc/c-family/c-attribs.c	(working copy)
@@ -4032,8 +4032,12 @@ validate_attribute (location_t atloc, tree oper, t
 
   if (TYPE_P (oper))
     tmpdecl = build_decl (atloc, TYPE_DECL, tmpid, oper);
+  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));
   else
-    tmpdecl = build_decl (atloc, TREE_CODE (oper), tmpid, TREE_TYPE (oper));
+    return false;
 
   /* Temporarily clear CURRENT_FUNCTION_DECL to make decl_attributes
      believe the DECL declared above is at file scope.  (See bug 87526.)  */
@@ -4042,7 +4046,7 @@ validate_attribute (location_t atloc, tree oper, t
   if (DECL_P (tmpdecl))
     {
       if (DECL_P (oper))
-	/* An alias cannot be a defintion so declare the symbol extern.  */
+	/* An alias cannot be a definition so declare the symbol extern.  */
 	DECL_EXTERNAL (tmpdecl) = true;
       /* Attribute visibility only applies to symbols visible from other
 	 translation units so make it "public."   */
@@ -4078,11 +4082,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
@@ -4133,7 +4143,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 268774)
+++ gcc/cp/parser.c	(working copy)
@@ -8542,9 +8542,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);
 
   STRIP_ANY_LOCATION_WRAPPER (oper);
 
Index: gcc/testsuite/c-c++-common/builtin-has-attribute-4.c
===================================================================
--- gcc/testsuite/c-c++-common/builtin-has-attribute-4.c	(revision 268774)
+++ gcc/testsuite/c-c++-common/builtin-has-attribute-4.c	(working copy)
@@ -154,7 +154,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)), ...))
@@ -164,7 +165,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,114 @@
+/* PR c/88383 - ICE calling _builtin_has_attribute(r, aligned(N)))
+   on an overaligned reference r
+   PR c/89288 - ICE in tree_code_size, at tree.c:865
+   { 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 doesn't confuse the attribute on
+     the pointer type with that to the pointed to type.  This
+     also exercises PR c/89288.  */
+  A (0, (Int8*)0, aligned);
+  A (0, (int*)0,  aligned);
+  A (0, (void*)0, aligned);
+  A (0, 0,        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] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Jakub Jelinek]

On Mon, Feb 11, 2019 at 12:20:42PM -0700, Martin Sebor wrote:
> This is a repost of a patch for PR 88383 updated to also fix the just
> reported PR 89288 (the original patch only partially handles this case).
> The review of the first patch was derailed by questions about the design
> of the built-in so the fix for the ICE was never approved.  I think
> the ICEs should be fixed for GCC 9 and any open design questions should
> be dealt with independently.

Well, it is closely coupled with the design questions.

>    if (TYPE_P (oper))
>      tmpdecl = build_decl (atloc, TYPE_DECL, tmpid, oper);
> +  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));
>    else
> -    tmpdecl = build_decl (atloc, TREE_CODE (oper), tmpid, TREE_TYPE (oper));
> +    return false;

The EXPR_P conditional makes no sense.  Why should __builtin_has_attribute (1 + 1, ...)
do something (if unfolded yet) and __builtin_has_attribute (2, ...)
something different?  1 + 1 when unfolded is EXPR_P, but 2 is not.

	Jakub

Re: [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Jakub Jelinek]

On Mon, Feb 11, 2019 at 12:20:42PM -0700, Martin Sebor wrote:
> --- gcc/c-family/c-attribs.c	(revision 268774)
> +++ gcc/c-family/c-attribs.c	(working copy)
> @@ -4032,8 +4032,12 @@ validate_attribute (location_t atloc, tree oper, t
>  
>    if (TYPE_P (oper))
>      tmpdecl = build_decl (atloc, TYPE_DECL, tmpid, oper);
> +  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));
>    else
> -    tmpdecl = build_decl (atloc, TREE_CODE (oper), tmpid, TREE_TYPE (oper));
> +    return false;
>  
>    /* Temporarily clear CURRENT_FUNCTION_DECL to make decl_attributes
>       believe the DECL declared above is at file scope.  (See bug 87526.)  */

Why do you this kind of validation at all?  You do compare the arguments
later on in the caller, why isn't that sufficient?  Creating some decl (and
ignoring for that whether the attribute is decl_required, type_required and/or
function_type_required) is a sure way to get many warnings, even if the
arguments are reasonable.

	Jakub

Re: [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

On 2/11/19 1:23 PM, Jakub Jelinek wrote:
> On Mon, Feb 11, 2019 at 12:20:42PM -0700, Martin Sebor wrote:
>> --- gcc/c-family/c-attribs.c	(revision 268774)
>> +++ gcc/c-family/c-attribs.c	(working copy)
>> @@ -4032,8 +4032,12 @@ validate_attribute (location_t atloc, tree oper, t
>>   
>>     if (TYPE_P (oper))
>>       tmpdecl = build_decl (atloc, TYPE_DECL, tmpid, oper);
>> +  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));
>>     else
>> -    tmpdecl = build_decl (atloc, TREE_CODE (oper), tmpid, TREE_TYPE (oper));
>> +    return false;
>>   
>>     /* Temporarily clear CURRENT_FUNCTION_DECL to make decl_attributes
>>        believe the DECL declared above is at file scope.  (See bug 87526.)  */
> 
> Why do you this kind of validation at all?  You do compare the arguments
> later on in the caller, why isn't that sufficient?  Creating some decl (and
> ignoring for that whether the attribute is decl_required, type_required and/or
> function_type_required) is a sure way to get many warnings, even if the
> arguments are reasonable.
> 

The snippet above deals with validating an attribute with one or
more arguments in the context where it's being queried, to make
sure the whole attribute specification makes any sense at all.
It's meant to catch gross mistakes like

   __builtin_has_attribute (func, alloc_size ("1"));

but it's far from perfect.

The difference in the example you asked about in your other mail
can be seen here:

   const int i = 1;

   enum {
     e = __builtin_has_attribute (1 + 0, alloc_size ("foo")),
     f = __builtin_has_attribute (i + 1, alloc_size ("bar"))
   };

Because the EPPR_P(oper) test fails for the first enumerator
the first invalid attribute specification is not diagnosed.  But
because it succeeds for (i + 1), the second specification triggers
a warning about alloc_size being only applicable to function types.
I suppose this could improved by handling constants the same as
expressions.

But this is far from the only limitation.  Attributes with no
arguments don't even make it this far.  Because the validation
depends on decl_attributes to detect these mistakes and that
function doesn't distinguish success from failure, the validation
succeeds even for non-sensical attributes.  But it should never
cause the built-in to give a false positive.  There are at least
a couple of FIXMEs in the code where I would like to improve
the validation in GCC 10.  But none of them is inherent in
the design of the built-in or serious enough to seriously
compromise its usefulness.

Martin

PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

Please let me know what it will take to get the fix for these two
issues approved.  I've answered the questions so I don't know what
else I'm expected to do here.

   https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html

On 2/11/19 12:20 PM, Martin Sebor wrote:
> This is a repost of a patch for PR 88383 updated to also fix the just
> reported PR 89288 (the original patch only partially handles this case).
> The review of the first patch was derailed by questions about the design
> of the built-in so the fix for the ICE was never approved.  I think
> the ICEs should be fixed for GCC 9 and any open design questions should
> be dealt with independently.
> 
> Martin
> 
> The patch for PR 88383 was originally posted last December:
>    https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Jeff Law]

On 2/18/19 7:53 PM, Martin Sebor wrote:
> Please let me know what it will take to get the fix for these two
> issues approved.  I've answered the questions so I don't know what
> else I'm expected to do here.
> 
>   https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html
I think there is still a fundamental disagreement about whether or not
this should be handling expressions.  Without an agreement on that it's
hard to see how this could go forward.

jeff

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

On 2/21/19 12:08 PM, Jeff Law wrote:
> On 2/18/19 7:53 PM, Martin Sebor wrote:
>> Please let me know what it will take to get the fix for these two
>> issues approved.  I've answered the questions so I don't know what
>> else I'm expected to do here.
>>
>>    https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html
> I think there is still a fundamental disagreement about whether or not
> this should be handling expressions.  Without an agreement on that it's
> hard to see how this could go forward.

I think it's wrong to hold up a fix for an ICE because you don't
like the current design.  The built-in successfully handles common
expressions (see c-c++-common/builtin-has-attribute-5.c).  It just
fails for some of the less common ones.  Not fixing those will only
penalize users who run into the ICE, and cast a bad light on
the quality of the release.   Any design questions should be
settled separately of these ICEs (should have been when
the feature was being reviewed).

That said, I have explained the rationale for the current design.
Neither you nor Jakub has explained what you find wrong with it or
why either of your alternatives is preferable.  You said it should
be an error to apply the built-in to expressions (why?).  Jakub
thought there perhaps should be two built-ins, one for expressions
and another for decls.  His rationale?  The current design is not
good.  Clearly,  the two of you don't agree on what you'd like to
see; the only thing you agree on is that you don't like what's
there now.  What do you expect me to do with that, now at the end
of stage 4?

Martin

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Jeff Law]

On 2/21/19 1:12 PM, Martin Sebor wrote:
> On 2/21/19 12:08 PM, Jeff Law wrote:
>> On 2/18/19 7:53 PM, Martin Sebor wrote:
>>> Please let me know what it will take to get the fix for these two
>>> issues approved.  I've answered the questions so I don't know what
>>> else I'm expected to do here.
>>>
>>>    https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html
>> I think there is still a fundamental disagreement about whether or not
>> this should be handling expressions.  Without an agreement on that it's
>> hard to see how this could go forward.
> 
> I think it's wrong to hold up a fix for an ICE because you don't
> like the current design.  The built-in successfully handles common
> expressions (see c-c++-common/builtin-has-attribute-5.c).  It just
> fails for some of the less common ones.  Not fixing those will only
> penalize users who run into the ICE, and cast a bad light on
> the quality of the release.   Any design questions should be
> settled separately of these ICEs (should have been when
> the feature was being reviewed).
> 
> That said, I have explained the rationale for the current design.
> Neither you nor Jakub has explained what you find wrong with it or
> why either of your alternatives is preferable.  You said it should
> be an error to apply the built-in to expressions (why?).  Jakub
> thought there perhaps should be two built-ins, one for expressions
> and another for decls.  His rationale?  The current design is not
> good.  Clearly,  the two of you don't agree on what you'd like to
> see; the only thing you agree on is that you don't like what's
> there now.  What do you expect me to do with that, now at the end
> of stage 4?
Fix the ice in another way.  Handling expressions here seems
fundamentally wrong.  ISTM that making this query on an expression
should result in an immediate error.

jeff

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

On 2/21/19 1:27 PM, Jeff Law wrote:
> On 2/21/19 1:12 PM, Martin Sebor wrote:
>> On 2/21/19 12:08 PM, Jeff Law wrote:
>>> On 2/18/19 7:53 PM, Martin Sebor wrote:
>>>> Please let me know what it will take to get the fix for these two
>>>> issues approved.  I've answered the questions so I don't know what
>>>> else I'm expected to do here.
>>>>
>>>>     https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html
>>> I think there is still a fundamental disagreement about whether or not
>>> this should be handling expressions.  Without an agreement on that it's
>>> hard to see how this could go forward.
>>
>> I think it's wrong to hold up a fix for an ICE because you don't
>> like the current design.  The built-in successfully handles common
>> expressions (see c-c++-common/builtin-has-attribute-5.c).  It just
>> fails for some of the less common ones.  Not fixing those will only
>> penalize users who run into the ICE, and cast a bad light on
>> the quality of the release.   Any design questions should be
>> settled separately of these ICEs (should have been when
>> the feature was being reviewed).
>>
>> That said, I have explained the rationale for the current design.
>> Neither you nor Jakub has explained what you find wrong with it or
>> why either of your alternatives is preferable.  You said it should
>> be an error to apply the built-in to expressions (why?).  Jakub
>> thought there perhaps should be two built-ins, one for expressions
>> and another for decls.  His rationale?  The current design is not
>> good.  Clearly,  the two of you don't agree on what you'd like to
>> see; the only thing you agree on is that you don't like what's
>> there now.  What do you expect me to do with that, now at the end
>> of stage 4?
> Fix the ice in another way.  Handling expressions here seems
> fundamentally wrong.  ISTM that making this query on an expression
> should result in an immediate error.

Sorry but "it seems wrong" is not a compelling argument.  You need
to explain what you think is wrong with it.

The built-in is modeled on the sizeof, alignof, and typeof operators.
The manual documents like this:

   bool __builtin_has_attribute (type-or-expression, attribute)

   ...The type-or-expression argument is subject to the same
   restrictions as the argument to typeof (see Typeof).

It was reviewed and approved with no objections to the API.  So it
seems that it's up to you to provide a convincing argument that this
was the wrong choice.  What serious problems do you think it might
cause that justify rejecting expressions, and how will users overcome
this limitation?

I already explained the rationale behind the decision to have
the built-in accept expressions: to be able to easily query for
type attributes in expressions such as:

   typedef __attribute__ ((may_alias)) int* BadInt;

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

or

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

   void f (struct S *p)
   {
     _Static_assert (__builtin_has_attribute (p->a, packed));
   }

or even

     _Static_assert (__builtin_has_attribute (p->a[0], packed));

If the built-in rejects expressions, I don't see how one would
query for these properties.

Martin

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Jeff Law]

On 2/21/19 3:39 PM, Martin Sebor wrote:
> On 2/21/19 1:27 PM, Jeff Law wrote:
>> On 2/21/19 1:12 PM, Martin Sebor wrote:
>>> On 2/21/19 12:08 PM, Jeff Law wrote:
>>>> On 2/18/19 7:53 PM, Martin Sebor wrote:
>>>>> Please let me know what it will take to get the fix for these two
>>>>> issues approved.  I've answered the questions so I don't know what
>>>>> else I'm expected to do here.
>>>>>
>>>>>     https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html
>>>> I think there is still a fundamental disagreement about whether or not
>>>> this should be handling expressions.  Without an agreement on that it's
>>>> hard to see how this could go forward.
>>>
>>> I think it's wrong to hold up a fix for an ICE because you don't
>>> like the current design.  The built-in successfully handles common
>>> expressions (see c-c++-common/builtin-has-attribute-5.c).  It just
>>> fails for some of the less common ones.  Not fixing those will only
>>> penalize users who run into the ICE, and cast a bad light on
>>> the quality of the release.   Any design questions should be
>>> settled separately of these ICEs (should have been when
>>> the feature was being reviewed).
>>>
>>> That said, I have explained the rationale for the current design.
>>> Neither you nor Jakub has explained what you find wrong with it or
>>> why either of your alternatives is preferable.  You said it should
>>> be an error to apply the built-in to expressions (why?).  Jakub
>>> thought there perhaps should be two built-ins, one for expressions
>>> and another for decls.  His rationale?  The current design is not
>>> good.  Clearly,  the two of you don't agree on what you'd like to
>>> see; the only thing you agree on is that you don't like what's
>>> there now.  What do you expect me to do with that, now at the end
>>> of stage 4?
>> Fix the ice in another way.  Handling expressions here seems
>> fundamentally wrong.  ISTM that making this query on an expression
>> should result in an immediate error.
> 
> Sorry but "it seems wrong" is not a compelling argument.  You need
> to explain what you think is wrong with it.
Attributes on expressions seems fundamentally broken.  Jakub has raised
this issue as well.  If you want things to move forward you need to
address this fundamental issue.

> 
> The built-in is modeled on the sizeof, alignof, and typeof operators.
> The manual documents like this:
Understood, but that makes it rather different from most (all?) other
attributes.  Attributes apply to decls and types, not expressions.

> 
>   bool __builtin_has_attribute (type-or-expression, attribute)
> 
>   ...The type-or-expression argument is subject to the same
>   restrictions as the argument to typeof (see Typeof).
> 
> It was reviewed and approved with no objections to the API. 

ANd I botched that.  Mistakes happen.


Jeff

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

On 2/21/19 3:42 PM, Jeff Law wrote:
> On 2/21/19 3:39 PM, Martin Sebor wrote:
>> On 2/21/19 1:27 PM, Jeff Law wrote:
>>> On 2/21/19 1:12 PM, Martin Sebor wrote:
>>>> On 2/21/19 12:08 PM, Jeff Law wrote:
>>>>> On 2/18/19 7:53 PM, Martin Sebor wrote:
>>>>>> Please let me know what it will take to get the fix for these two
>>>>>> issues approved.  I've answered the questions so I don't know what
>>>>>> else I'm expected to do here.
>>>>>>
>>>>>>      https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html
>>>>> I think there is still a fundamental disagreement about whether or not
>>>>> this should be handling expressions.  Without an agreement on that it's
>>>>> hard to see how this could go forward.
>>>>
>>>> I think it's wrong to hold up a fix for an ICE because you don't
>>>> like the current design.  The built-in successfully handles common
>>>> expressions (see c-c++-common/builtin-has-attribute-5.c).  It just
>>>> fails for some of the less common ones.  Not fixing those will only
>>>> penalize users who run into the ICE, and cast a bad light on
>>>> the quality of the release.   Any design questions should be
>>>> settled separately of these ICEs (should have been when
>>>> the feature was being reviewed).
>>>>
>>>> That said, I have explained the rationale for the current design.
>>>> Neither you nor Jakub has explained what you find wrong with it or
>>>> why either of your alternatives is preferable.  You said it should
>>>> be an error to apply the built-in to expressions (why?).  Jakub
>>>> thought there perhaps should be two built-ins, one for expressions
>>>> and another for decls.  His rationale?  The current design is not
>>>> good.  Clearly,  the two of you don't agree on what you'd like to
>>>> see; the only thing you agree on is that you don't like what's
>>>> there now.  What do you expect me to do with that, now at the end
>>>> of stage 4?
>>> Fix the ice in another way.  Handling expressions here seems
>>> fundamentally wrong.  ISTM that making this query on an expression
>>> should result in an immediate error.
>>
>> Sorry but "it seems wrong" is not a compelling argument.  You need
>> to explain what you think is wrong with it.
> Attributes on expressions seems fundamentally broken.  Jakub has raised
> this issue as well.  If you want things to move forward you need to
> address this fundamental issue.

I need to understand what this fundamental problem is.

>> The built-in is modeled on the sizeof, alignof, and typeof operators.
>> The manual documents like this:
> Understood, but that makes it rather different from most (all?) other
> attributes.  Attributes apply to decls and types, not expressions.
> 
>>
>>    bool __builtin_has_attribute (type-or-expression, attribute)
>>
>>    ...The type-or-expression argument is subject to the same
>>    restrictions as the argument to typeof (see Typeof).
>>
>> It was reviewed and approved with no objections to the API.
> 
> ANd I botched that.  Mistakes happen.

It was also reviewed by Joseph and Jason.

Can you show how one would query for attribute packed in the example
I gave if the built-in were to reject expressions:

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

   void f (struct S *p)
   {
     _Static_assert (__builtin_has_attribute (p->a, packed));
   }

In any event, if there is a problem I will certainly fix it.  But
I need to know what the problem is first.

Martin

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Marek Polacek]

On Thu, Feb 21, 2019 at 03:39:21PM -0700, Martin Sebor wrote:
> On 2/21/19 1:27 PM, Jeff Law wrote:
> > On 2/21/19 1:12 PM, Martin Sebor wrote:
> > > On 2/21/19 12:08 PM, Jeff Law wrote:
> > > > On 2/18/19 7:53 PM, Martin Sebor wrote:
> > > > > Please let me know what it will take to get the fix for these two
> > > > > issues approved.  I've answered the questions so I don't know what
> > > > > else I'm expected to do here.
> > > > > 
> > > > >     https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html
> > > > I think there is still a fundamental disagreement about whether or not
> > > > this should be handling expressions.  Without an agreement on that it's
> > > > hard to see how this could go forward.
> > > 
> > > I think it's wrong to hold up a fix for an ICE because you don't
> > > like the current design.  The built-in successfully handles common
> > > expressions (see c-c++-common/builtin-has-attribute-5.c).  It just
> > > fails for some of the less common ones.  Not fixing those will only
> > > penalize users who run into the ICE, and cast a bad light on
> > > the quality of the release.   Any design questions should be
> > > settled separately of these ICEs (should have been when
> > > the feature was being reviewed).
> > > 
> > > That said, I have explained the rationale for the current design.
> > > Neither you nor Jakub has explained what you find wrong with it or
> > > why either of your alternatives is preferable.  You said it should
> > > be an error to apply the built-in to expressions (why?).  Jakub
> > > thought there perhaps should be two built-ins, one for expressions
> > > and another for decls.  His rationale?  The current design is not
> > > good.  Clearly,  the two of you don't agree on what you'd like to
> > > see; the only thing you agree on is that you don't like what's
> > > there now.  What do you expect me to do with that, now at the end
> > > of stage 4?
> > Fix the ice in another way.  Handling expressions here seems
> > fundamentally wrong.  ISTM that making this query on an expression
> > should result in an immediate error.
> 
> Sorry but "it seems wrong" is not a compelling argument.  You need
> to explain what you think is wrong with it.

"Attributes apply to decls and types, not expressions" seems like an
argument strong enough.  (There are also special attributes for enums
 and ';' and labels.)

> The built-in is modeled on the sizeof, alignof, and typeof operators.
> The manual documents like this:
> 
>   bool __builtin_has_attribute (type-or-expression, attribute)
> 
>   ...The type-or-expression argument is subject to the same
>   restrictions as the argument to typeof (see Typeof).
> 
> It was reviewed and approved with no objections to the API.  So it
> seems that it's up to you to provide a convincing argument that this
> was the wrong choice.  What serious problems do you think it might
> cause that justify rejecting expressions, and how will users overcome
> this limitation?
> 
> I already explained the rationale behind the decision to have
> the built-in accept expressions: to be able to easily query for
> type attributes in expressions such as:
> 
>   typedef __attribute__ ((may_alias)) int* BadInt;
> 
>   void f (BadInt *p)
>   {
>     _Static_assert (__builtin_has_attribute (*p, may_alias));
>   }
> 
> or
> 
>   struct S {
>     int a[1] __attribute__ ((packed));
>   };
> 
>   void f (struct S *p)
>   {
>     _Static_assert (__builtin_has_attribute (p->a, packed));
>   }
> 
> or even
> 
>     _Static_assert (__builtin_has_attribute (p->a[0], packed));
> 
> If the built-in rejects expressions, I don't see how one would
> query for these properties.

So how about __builtin_has_attribute (typeof (p->a), packed)?

Trying to query/set attributes on things like "1 + 1" just doesn't make any
sense.  So why don't we handle TYPE_P/DECL_P and give an error for the rest?

Marek

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

>> I already explained the rationale behind the decision to have
>> the built-in accept expressions: to be able to easily query for
>> type attributes in expressions such as:
>>
>>    typedef __attribute__ ((may_alias)) int* BadInt;
>>
>>    void f (BadInt *p)
>>    {
>>      _Static_assert (__builtin_has_attribute (*p, may_alias));
>>    }
>>
>> or
>>
>>    struct S {
>>      int a[1] __attribute__ ((packed));
>>    };
>>
>>    void f (struct S *p)
>>    {
>>      _Static_assert (__builtin_has_attribute (p->a, packed));
>>    }
>>
>> or even
>>
>>      _Static_assert (__builtin_has_attribute (p->a[0], packed));
>>
>> If the built-in rejects expressions, I don't see how one would
>> query for these properties.
> 
> So how about __builtin_has_attribute (typeof (p->a), packed)?

typeof (p->a) is int, not 'packed int', so the built-in returns
false in this case, while true in the expression I wrote.  This
applies to any attribute that's attached to a member as opposed
to its type.  I know of no way to refer to a struct member in C
without using some non-trivial expression (in C++, S::a could
be used instead).

> Trying to query/set attributes on things like "1 + 1" just doesn't make any
> sense.  So why don't we handle TYPE_P/DECL_P and give an error for the rest?

Mainly because of the above.  But also because there is no harm
in accepting arbitrary expressions and querying their type, no
problem that I'm aware of that would justify giving an error.

This is analogous to __alignof__.  If __alignof__ (1 + 1) makes
enough sense to accept then I see no reason why
__builtin_has_attribute (1 + 1, aligned) should not be.

Martin

PS __alignof__ is documented to determine the alignment requirement
of a function, object, or a type, but it accepts expressions as well.
For lvalues, like p->a above, __alignof__ is further documented to
determine the alignment of the lvalue's type, but that's not what
it actually does, at least not in the sense of
__alignof__ (__typeof__ (p->a)), for over- or underaligned struct
members declared either with attribute aligned or packed.  What
it does is return the alignment of the member subobject, which is,
I'm sure, what most users expect.

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Jeff Law]

On 2/22/19 9:42 AM, Marek Polacek wrote:
> On Thu, Feb 21, 2019 at 03:39:21PM -0700, Martin Sebor wrote:
>> On 2/21/19 1:27 PM, Jeff Law wrote:
>>> On 2/21/19 1:12 PM, Martin Sebor wrote:
>>>> On 2/21/19 12:08 PM, Jeff Law wrote:
>>>>> On 2/18/19 7:53 PM, Martin Sebor wrote:
>>>>>> Please let me know what it will take to get the fix for these two
>>>>>> issues approved.  I've answered the questions so I don't know what
>>>>>> else I'm expected to do here.
>>>>>>
>>>>>>     https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html
>>>>> I think there is still a fundamental disagreement about whether or not
>>>>> this should be handling expressions.  Without an agreement on that it's
>>>>> hard to see how this could go forward.
>>>>
>>>> I think it's wrong to hold up a fix for an ICE because you don't
>>>> like the current design.  The built-in successfully handles common
>>>> expressions (see c-c++-common/builtin-has-attribute-5.c).  It just
>>>> fails for some of the less common ones.  Not fixing those will only
>>>> penalize users who run into the ICE, and cast a bad light on
>>>> the quality of the release.   Any design questions should be
>>>> settled separately of these ICEs (should have been when
>>>> the feature was being reviewed).
>>>>
>>>> That said, I have explained the rationale for the current design.
>>>> Neither you nor Jakub has explained what you find wrong with it or
>>>> why either of your alternatives is preferable.  You said it should
>>>> be an error to apply the built-in to expressions (why?).  Jakub
>>>> thought there perhaps should be two built-ins, one for expressions
>>>> and another for decls.  His rationale?  The current design is not
>>>> good.  Clearly,  the two of you don't agree on what you'd like to
>>>> see; the only thing you agree on is that you don't like what's
>>>> there now.  What do you expect me to do with that, now at the end
>>>> of stage 4?
>>> Fix the ice in another way.  Handling expressions here seems
>>> fundamentally wrong.  ISTM that making this query on an expression
>>> should result in an immediate error.
>>
>> Sorry but "it seems wrong" is not a compelling argument.  You need
>> to explain what you think is wrong with it.
> 
> "Attributes apply to decls and types, not expressions" seems like an
> argument strong enough.  (There are also special attributes for enums
>  and ';' and labels.)
So I think Martin's argument is that for an expression we can look at
the type of the expression and see if the attribute is on that type.

That leads to some inconsistencies (that Jakub has pointed out) where an
unfolded expression could produce a different result than it's folded
result if the result was a constant.

AFAICT the goal (outside handling just types) here is to be able to ask
things like is a particular field within a structure packed, aliasing
properties of objects, etc.

My next thought was to use typeof to extract the type of the expression
and pass that through (independent of Marek suggesting the same).
Martin points out in a subsequent email that won't work because p->a in
the supplied example returns an int rather than a packed int.   One
could argue that's the wrong return value for typeof, but I don't think
it's that clear cut.  In this specific instance I'd claim it's wrong,
but I'd hazard a guess we don't have clear semantics in this space.

I'll note that our documentation clearly states that attributes can be
applied to functions, variables, labels, enums, statements and types.
No provision AFAICT is made for expressions.  Users have never had the
ability or assumption that they can ask for attributes on an expression.

Martin's patch essentially allows for asking indirectly -- but I suspect
it's going to be inconsistent in more ways than Jakub pointed out as the
type system in gimple is "loose" in certain ways and the result could
well change as we go through the optimizer pipeline (ie, this is bigger
than just constant folding causing inconsistencies).

Given that we're talking about a new feature with no existing end user
expectations and the fact that attributes are documented as applying
only to things which are decls or types, I'm more inclined to stay
conservative at this point and reject expressions as syntax errors.

We can revisit in gcc-10 and see if there's a reasonable way to tighten
semantics around typeof and/or loosen the requirement that we can only
ask about attributes of DECL/TYPE nodes and define good semantics around
that if we try.  I'm also willing to revisit if other reviewers chime in
with other opinions -- this kind of stuff isn't my strongest area.


Jeff

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

On 3/19/19 12:30 PM, Jeff Law wrote:
> On 2/22/19 9:42 AM, Marek Polacek wrote:
>> On Thu, Feb 21, 2019 at 03:39:21PM -0700, Martin Sebor wrote:
>>> On 2/21/19 1:27 PM, Jeff Law wrote:
>>>> On 2/21/19 1:12 PM, Martin Sebor wrote:
>>>>> On 2/21/19 12:08 PM, Jeff Law wrote:
>>>>>> On 2/18/19 7:53 PM, Martin Sebor wrote:
>>>>>>> Please let me know what it will take to get the fix for these two
>>>>>>> issues approved.  I've answered the questions so I don't know what
>>>>>>> else I'm expected to do here.
>>>>>>>
>>>>>>>      https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html
>>>>>> I think there is still a fundamental disagreement about whether or not
>>>>>> this should be handling expressions.  Without an agreement on that it's
>>>>>> hard to see how this could go forward.
>>>>>
>>>>> I think it's wrong to hold up a fix for an ICE because you don't
>>>>> like the current design.  The built-in successfully handles common
>>>>> expressions (see c-c++-common/builtin-has-attribute-5.c).  It just
>>>>> fails for some of the less common ones.  Not fixing those will only
>>>>> penalize users who run into the ICE, and cast a bad light on
>>>>> the quality of the release.   Any design questions should be
>>>>> settled separately of these ICEs (should have been when
>>>>> the feature was being reviewed).
>>>>>
>>>>> That said, I have explained the rationale for the current design.
>>>>> Neither you nor Jakub has explained what you find wrong with it or
>>>>> why either of your alternatives is preferable.  You said it should
>>>>> be an error to apply the built-in to expressions (why?).  Jakub
>>>>> thought there perhaps should be two built-ins, one for expressions
>>>>> and another for decls.  His rationale?  The current design is not
>>>>> good.  Clearly,  the two of you don't agree on what you'd like to
>>>>> see; the only thing you agree on is that you don't like what's
>>>>> there now.  What do you expect me to do with that, now at the end
>>>>> of stage 4?
>>>> Fix the ice in another way.  Handling expressions here seems
>>>> fundamentally wrong.  ISTM that making this query on an expression
>>>> should result in an immediate error.
>>>
>>> Sorry but "it seems wrong" is not a compelling argument.  You need
>>> to explain what you think is wrong with it.
>>
>> "Attributes apply to decls and types, not expressions" seems like an
>> argument strong enough.  (There are also special attributes for enums
>>   and ';' and labels.)
> So I think Martin's argument is that for an expression we can look at
> the type of the expression and see if the attribute is on that type.
> 
> That leads to some inconsistencies (that Jakub has pointed out) where an
> unfolded expression could produce a different result than it's folded
> result if the result was a constant.

The built-in is evaluated during parsing (same as __alignof__)
so I'm not sure I understand what inconsistencies it could be
subject to that __alignof__ isn't.  I'm open to examples, I just
can't think of any.

> 
> AFAICT the goal (outside handling just types) here is to be able to ask
> things like is a particular field within a structure packed, aliasing
> properties of objects, etc.

Yes.  The goal is to query functions, variables (including members),
and types for most attributes known to GCC that may be attached to
them.

> 
> My next thought was to use typeof to extract the type of the expression
> and pass that through (independent of Marek suggesting the same).
> Martin points out in a subsequent email that won't work because p->a in
> the supplied example returns an int rather than a packed int.   One
> could argue that's the wrong return value for typeof, but I don't think
> it's that clear cut.  In this specific instance I'd claim it's wrong,
> but I'd hazard a guess we don't have clear semantics in this space.

__alignof__ is documented to work this way:

   If the operand of __alignof__ is an lvalue rather than a type,
   its value is the required alignment for its type, taking into
   account any minimum alignment specified with GCC's __attribute__
   extension (see Variable Attributes). For example, after this
   declaration:

      struct foo { int x; char y; } foo1;

   the value of __alignof__ (foo1.y) is 1, even though its actual
   alignment is probably 2 or 4, the same as __alignof__ (int).

What isn't documented is the semantics for expressions that aren't
lvalues.  They are nevertheless accepted and, AFAICT, __alignof__
returns the alignment of their type.

> 
> I'll note that our documentation clearly states that attributes can be
> applied to functions, variables, labels, enums, statements and types.
> No provision AFAICT is made for expressions.  Users have never had the
> ability or assumption that they can ask for attributes on an expression.

Not quite.  As I said above, __alignof__ accepts expressions
because there is no way to apply the operator to a member without
using an expression of some sort.  But it also accepts any other
expressions.

> Martin's patch essentially allows for asking indirectly -- but I suspect
> it's going to be inconsistent in more ways than Jakub pointed out as the
> type system in gimple is "loose" in certain ways and the result could
> well change as we go through the optimizer pipeline (ie, this is bigger
> than just constant folding causing inconsistencies).

As I explained above, the built-in is fully evaluated and folded
by the front-ends during parsing.  It never makes it into GIMPLE.

> 
> Given that we're talking about a new feature with no existing end user
> expectations and the fact that attributes are documented as applying
> only to things which are decls or types, I'm more inclined to stay
> conservative at this point and reject expressions as syntax errors.
> 
> We can revisit in gcc-10 and see if there's a reasonable way to tighten
> semantics around typeof and/or loosen the requirement that we can only
> ask about attributes of DECL/TYPE nodes and define good semantics around
> that if we try.  I'm also willing to revisit if other reviewers chime in
> with other opinions -- this kind of stuff isn't my strongest area.

There is no way to do what you suggest without preventing
the built-in from being used with struct members, and  I don't
see what good crippling it like that would do, or what could
then be done differently to fix it.

By the way of an example, both the __alignof__ and the built-in
arguments are accepted today but the latter would not work if
references to members were considered an error:

   struct S { __attribute__ ((packed, aligned (2))) int i; };

   _Static_assert (__alignof__ (((struct S*)0)->i) == 2);
   _Static_assert (__alignof__ (__typeof__ (((struct S*)0)->i)) == 4);

   _Static_assert (__builtin_has_attribute (((struct S*)0)->i, packed));
   _Static_assert (__builtin_has_attribute (((struct S*)0)->i, aligned));

   _Static_assert (!__builtin_has_attribute (__typeof__ (((struct 
S*)0)->i), packed));
   _Static_assert (!__builtin_has_attribute (__typeof__ (((struct 
S*)0)->i), aligned));

because the attribute is attached to i's DECL and not to its type,
and there is no other way to refer to the member i of struct S.

Martin

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Joseph Myers]

On Tue, 19 Mar 2019, Jeff Law wrote:

> I'll note that our documentation clearly states that attributes can be
> applied to functions, variables, labels, enums, statements and types.

A key thing here is that they can be applied to fields - that is, they can 
be properties of a FIELD_DECL.  Referring to a field either requires an 
expression referring to that field, or some other custom syntax that uses 
both a type name and a field name (like in __builtin_offsetof).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Jeff Law]

On 3/19/19 8:22 PM, Joseph Myers wrote:
> On Tue, 19 Mar 2019, Jeff Law wrote:
> 
>> I'll note that our documentation clearly states that attributes can be
>> applied to functions, variables, labels, enums, statements and types.
> 
> A key thing here is that they can be applied to fields - that is, they can 
> be properties of a FIELD_DECL.  Referring to a field either requires an 
> expression referring to that field, or some other custom syntax that uses 
> both a type name and a field name (like in __builtin_offsetof).
> 
Thanks for chiming in, your opinions on this kind of thing are greatly
appreciated.

Understood WRT applying to fields, and I think that's consistent with
some of what Jakub has expressed elsewhere -- specifically that we
should consider allowing COMPONENT_REFs as the exception, returning the
attributes of the associated FIELD_DECL in that case.

Is there a need to call out BIT_FIELD_REFs where we can't actually get
to the underlying DECL?  And if so, how do we do that in the end user
documentation that is clear and consistent?

One of the big worries I've got here is that documenting when an
expression as an argument to __builtin_has_attribute (or any attribute
query) can be expected to work.  It's always easier on our end users to
loosen semantics of extensions over time than it is to tighten them.


Jeff

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

On 3/19/19 9:33 PM, Jeff Law wrote:
> On 3/19/19 8:22 PM, Joseph Myers wrote:
>> On Tue, 19 Mar 2019, Jeff Law wrote:
>>
>>> I'll note that our documentation clearly states that attributes can be
>>> applied to functions, variables, labels, enums, statements and types.
>>
>> A key thing here is that they can be applied to fields - that is, they can
>> be properties of a FIELD_DECL.  Referring to a field either requires an
>> expression referring to that field, or some other custom syntax that uses
>> both a type name and a field name (like in __builtin_offsetof).
>>
> Thanks for chiming in, your opinions on this kind of thing are greatly
> appreciated.
> 
> Understood WRT applying to fields, and I think that's consistent with
> some of what Jakub has expressed elsewhere -- specifically that we
> should consider allowing COMPONENT_REFs as the exception, returning the
> attributes of the associated FIELD_DECL in that case.
> 
> Is there a need to call out BIT_FIELD_REFs where we can't actually get
> to the underlying DECL?  And if so, how do we do that in the end user
> documentation that is clear and consistent?

Is it possible to see a BIT_FIELD_REF here?  I couldn't find a way.

> 
> One of the big worries I've got here is that documenting when an
> expression as an argument to __builtin_has_attribute (or any attribute
> query) can be expected to work.  It's always easier on our end users to
> loosen semantics of extensions over time than it is to tighten them.

I wonder if a part of the issue isn't due to a mismatch between
the C terminology and what GCC uses internally.  Every argument
to the built-in that's not a type (and every argument to attribute
copy which doesn't accept types) must be a C expression:

1) either an identifier naming a function or variable, or
2) some other expression like a member reference via . or ->,
    an array subscript, or the indirection expression *.

But GCC distinguishes three kinds of arguments:

1) a DECL,
2) some sort of a reference like ARRAY_REF, COMPONENT_REF or
    INDIRECT_REF
3) an expression that satisfies the EXPR_P() predicate (e.g.,
    (struct S*)0, or (struct S){ 1 })

Jeff, you seem to want the built-in to accept just (1) on the GCC
list above and reject (3) (and seem to be waffling on (2)).

How would such an argument be described in a way that users
unfamiliar with GCC internals could easily understand?

All sorts of expressions can be used to refer to fields.  Given
the type definition 'struct A { int b[2]; };' besides
FUNCTION_DECL, PARM_DECL, and VAR_DECL we might expect to commonly
see arguments like:

COMPONENT_REF:
   ((struct A*)0)->b
   (*((struct A*)0)).b
   ((struct A*)0)[0].b

INDIRECT_REF:
   *((struct A*)0)[0].b

ARRAY_REF:
   ((struct A*)0)[0].b[0]

To users, they're all just expressions.

Fields aside, the expressions below seem quite plausible to me
too, especially when the built-in argument is the result of macro
expansion:

CALL_EXPR:
   foo ()

COMPOUND_LITERAL_EXPR:
   (struct A){ ... }

COND_EXPR:
   (0 ? a0 : a1)

COMPOUND_EXPR:
   ((void)0, a)

NON_LVALUE_EXPR:
   (struct A)a

etc.  In these cases the built-in could be passed the result of
__typeof__ instead, but if the built-in itself is part of a macro
that can be invoked either with one of the arguments on the _REF
list or with one of these expressions, it would only work as
expected for half of them.

Using __typeof__ doesn't work for the copy attribute because it
only takes expressions as arguments and not types.

Martin

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Jakub Jelinek]

On Thu, Mar 21, 2019 at 03:59:55PM -0600, Martin Sebor wrote:
> 1) either an identifier naming a function or variable, or
> 2) some other expression like a member reference via . or ->,
>    an array subscript, or the indirection expression *.
> 
> But GCC distinguishes three kinds of arguments:
> 
> 1) a DECL,
> 2) some sort of a reference like ARRAY_REF, COMPONENT_REF or
>    INDIRECT_REF
> 3) an expression that satisfies the EXPR_P() predicate (e.g.,
>    (struct S*)0, or (struct S){ 1 })
> 
> Jeff, you seem to want the built-in to accept just (1) on the GCC
> list above and reject (3) (and seem to be waffling on (2)).
> 
> How would such an argument be described in a way that users
> unfamiliar with GCC internals could easily understand?

Say that the argument is either a type or an expression that is
either an identifier (for C++ id-expression) to cover 1) and
a postfix expression with . or -> operator (to cover COMPONENT_REF)?
We do not want to allow INDIRECT_REF, ARRAY_REF, etc.

	Jakub

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

On 3/21/19 4:13 PM, Jakub Jelinek wrote:
> On Thu, Mar 21, 2019 at 03:59:55PM -0600, Martin Sebor wrote:
>> 1) either an identifier naming a function or variable, or
>> 2) some other expression like a member reference via . or ->,
>>     an array subscript, or the indirection expression *.
>>
>> But GCC distinguishes three kinds of arguments:
>>
>> 1) a DECL,
>> 2) some sort of a reference like ARRAY_REF, COMPONENT_REF or
>>     INDIRECT_REF
>> 3) an expression that satisfies the EXPR_P() predicate (e.g.,
>>     (struct S*)0, or (struct S){ 1 })
>>
>> Jeff, you seem to want the built-in to accept just (1) on the GCC
>> list above and reject (3) (and seem to be waffling on (2)).
>>
>> How would such an argument be described in a way that users
>> unfamiliar with GCC internals could easily understand?
> 
> Say that the argument is either a type or an expression that is
> either an identifier (for C++ id-expression) to cover 1) and
> a postfix expression with . or -> operator (to cover COMPONENT_REF)?

That doesn't look easy to understand.

> We do not want to allow INDIRECT_REF, ARRAY_REF, etc.

Why not?  What exactly is the concern?

Martin

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Jakub Jelinek]

On Thu, Mar 21, 2019 at 04:19:37PM -0600, Martin Sebor wrote:
> > Say that the argument is either a type or an expression that is
> > either an identifier (for C++ id-expression) to cover 1) and
> > a postfix expression with . or -> operator (to cover COMPONENT_REF)?
> 
> That doesn't look easy to understand.

Why?  Those users that don't read corresponding language standards
will just see the compiler diagnosing any uses but those 3 kinds
and can then just read the documentation which will show in examples what is
accepted and what it will do.

> > We do not want to allow INDIRECT_REF, ARRAY_REF, etc.
> 
> Why not?  What exactly is the concern?

Because only the . and -> operators are needed to get at the non-static
member declaration.  INDIRECT_REF or ARRAY_REF aren't useful for that,
and then it raises the question what exactly is supposed to be the behavior
when you use *expr or expr[0] or expr->field[0].  Those expression don't
have any decl, so we'd be back at your suggestion to pretty randomly
sometimes return DECL_ATTRIBUTES, sometimes TYPE_ATTRIBUTES, sometimes both.
That is just a bad design.  If people want type attributes, they should use
__typeof or decltype or just type itself in the argument.

	Jakub

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

On 3/21/19 4:25 PM, Jakub Jelinek wrote:
> On Thu, Mar 21, 2019 at 04:19:37PM -0600, Martin Sebor wrote:
>>> Say that the argument is either a type or an expression that is
>>> either an identifier (for C++ id-expression) to cover 1) and
>>> a postfix expression with . or -> operator (to cover COMPONENT_REF)?
>>
>> That doesn't look easy to understand.
> 
> Why?  Those users that don't read corresponding language standards
> will just see the compiler diagnosing any uses but those 3 kinds
> and can then just read the documentation which will show in examples what is
> accepted and what it will do.

Because to most users it suggests that . and -> are the only operators
that are accepted in the expression and so something like p[0].a is not
a valid argument.

Those who don't read the manual will be puzzled when, after having had
p[0].a accepted, p->a[0] will trigger an error.

>>> We do not want to allow INDIRECT_REF, ARRAY_REF, etc.
>>
>> Why not?  What exactly is the concern?
> 
> Because only the . and -> operators are needed to get at the non-static
> member declaration.  INDIRECT_REF or ARRAY_REF aren't useful for that,
> and then it raises the question what exactly is supposed to be the behavior
> when you use *expr or expr[0] or expr->field[0].  Those expression don't
> have any decl, so we'd be back at your suggestion to pretty randomly
> sometimes return DECL_ATTRIBUTES, sometimes TYPE_ATTRIBUTES, sometimes both.
> That is just a bad design.  If people want type attributes, they should use
> __typeof or decltype or just type itself in the argument.

I never suggested to "pretty randomly sometimes return
DECL_ATTRIBUTES, sometimes TYPE_ATTRIBUTES, or sometimes both."
That's a silly mischaracterization, just as it would be to say
the same thing about __alignof__, and it works much the same way.
It operates on expressions and returns the alignment either
explicitly speciified for the variable (or function) or that
specified for its type.

I modeled the built-in on __alignof__.  There may bugs where it
behaves differently, or subtle deviations where I chose a different
approach, but it's certainly not random.  I'd like to fix the bugs,
and I'm open to reconsidering the deviations, but using either as
a justification for rejecting those kinds of expressions would
result in hard-to-use API.  And _that_ would be bad design.

Martin

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Martin Sebor]

On 3/21/19 5:05 PM, Martin Sebor wrote:
> On 3/21/19 4:25 PM, Jakub Jelinek wrote:
>> On Thu, Mar 21, 2019 at 04:19:37PM -0600, Martin Sebor wrote:
>>>> Say that the argument is either a type or an expression that is
>>>> either an identifier (for C++ id-expression) to cover 1) and
>>>> a postfix expression with . or -> operator (to cover COMPONENT_REF)?
>>>
>>> That doesn't look easy to understand.
>>
>> Why?  Those users that don't read corresponding language standards
>> will just see the compiler diagnosing any uses but those 3 kinds
>> and can then just read the documentation which will show in examples 
>> what is
>> accepted and what it will do.
> 
> Because to most users it suggests that . and -> are the only operators
> that are accepted in the expression and so something like p[0].a is not
> a valid argument.
> 
> Those who don't read the manual will be puzzled when, after having had
> p[0].a accepted, p->a[0] will trigger an error.
> 
>>>> We do not want to allow INDIRECT_REF, ARRAY_REF, etc.
>>>
>>> Why not?  What exactly is the concern?
>>
>> Because only the . and -> operators are needed to get at the non-static
>> member declaration.  INDIRECT_REF or ARRAY_REF aren't useful for that,
>> and then it raises the question what exactly is supposed to be the 
>> behavior
>> when you use *expr or expr[0] or expr->field[0].  Those expression don't
>> have any decl, so we'd be back at your suggestion to pretty randomly
>> sometimes return DECL_ATTRIBUTES, sometimes TYPE_ATTRIBUTES, sometimes 
>> both.
>> That is just a bad design.  If people want type attributes, they 
>> should use
>> __typeof or decltype or just type itself in the argument.
> 
> I never suggested to "pretty randomly sometimes return
> DECL_ATTRIBUTES, sometimes TYPE_ATTRIBUTES, or sometimes both."
> That's a silly mischaracterization, just as it would be to say
> the same thing about __alignof__, and it works much the same way.
> It operates on expressions and returns the alignment either
> explicitly speciified for the variable (or function) or that
> specified for its type.
> 
> I modeled the built-in on __alignof__.  There may bugs where it
> behaves differently, or subtle deviations where I chose a different
> approach, but it's certainly not random.  I'd like to fix the bugs,
> and I'm open to reconsidering the deviations, but using either as
> a justification for rejecting those kinds of expressions would
> result in hard-to-use API.  And _that_ would be bad design.

An example of a deviation from __alignof__ that comes to mind
is this:

   __attribute__ ((aligned (8))) int a[2];

   _Static_assert (__alignof__ (a) == 8);
   _Static_assert (__builtin_has_attribute (a, aligned));

   _Static_assert (a[0]) == 8);   // fails
   _Static_assert (__builtin_has_attribute (a[0], aligned));

I made the ARRAY_REF case the same as the VAR_DECL case because
the alignment of a whole array also implies the alignment of
the array's elements.  Ditto for attribute packed. (And with
the patch the INDIRECT_REF has the same effect.)

I'm guessing that what you're objecting to is that this decision
conflates the results of the builtin for an array declared with
an attribute with that of the array's elements, as in:

   __attribute__ ((vector_size (8))) int a[2];

   _Static_assert (__builtin_has_attribute (a, vector_size));
   _Static_assert (__builtin_has_attribute (a[0], vector_size));

where both assertions pass, even though just the array element
is a vector, while a itself is an ordinary array.  I can see
that is not quite right and might warrant reconsidering
the decision for ARRAY_REF.

To your point about using __typeof__: GCC is inconsistent about
what it applies variable and function attributes to, whether
the decl or its type.  For example, vector_size is documented
as a variable attribute but it applies to a type.  Similarly,
attributes alloc_size and malloc are both documented as
function attributes but the former applies to the function
type (and so can be applied to a pointer to function), while
the latter to the function decl (and is ignored with
a warning on pointers to functions).  Until this is cleaned
up, either by correcting the documentation, or preferably by
making these attributes uniformly apply to types, it should
be obvious that suggesting that "if people want type attributes,
they should use __typeof or decltype" does not offer a viable
solution.

Martin

[PATCH] fix ICEs in c-attribs.c (PR 88383, 89288, 89798, 89797)​

[Martin Sebor]

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

This is yet another follow up on the discussion of the fix for
the ICE in __builtin_has_attribute that started last December
with PR88383.

After my last post last week I went and added more tests to make
sure the built-in behaves as intended by comparing its result for
non-trivial expressions with that of __alignof__.  The test that
does this is the new builtin-has-attribute-7.c.

The test exposed one problem in the handling of attribute vector_size
by the built-in (I mentioned that in my last post).  It also exposed
a couple of bugs in the attribute handler itself.  I fixed both of
these in the attached patch.

This latest revision of the patch resolves the following bugs:

   PR 88383 - ICE calling __builtin_has_attribute on a reference
   PR 89288 - ICE in tree_code_size, at tree.c:865
   PR 89798 - excessive vector_size silently accepted and truncated
   PR 89797 - ICE on a vector_size (1LU << 33) int variable

Bootstrapped on x86_64-linux.  The tests are still running but
assuming they pass, is this last revision good to commit?

Martin

A link to my last comment in the archive:
   https://gcc.gnu.org/ml/gcc-patches/2019-03/msg01096.html

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

PR c/88383 - ICE calling __builtin_has_attribute on a reference
PR c/89288 - ICE in tree_code_size, at tree.c:865
PR c/89798 - excessive vector_size silently accepted and truncated
PR c/89797 - ICE on a vector_size (1LU << 33) int variable

gcc/ChangeLog:

	PR c/89797
	* targhooks.c (default_vector_alignment): Avoid assuming
	argument fits in SHWI.
	* tree.h (TYPE_VECTOR_SUBPARTS): Avoid sign overflow in
	a shift expression.

gcc/c-family/ChangeLog:

	PR c/88383
	PR c/89288
	PR c/89798
	PR c/89797
	* c-attribs.c (type_valid_for_vector_size): Detect excessively
	large sizes.
	(validate_attribute): Handle DECLs and expressions.
	(has_attribute): Handle types referenced by expressions.
	Avoid considering array attributes in ARRAY_REF expressions .

gcc/cp/ChangeLog:

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

gcc/testsuite/ChangeLog:

	PR c/88383
	PR c/89288
	PR c/89798
	PR c/89797
	* c-c++-common/attributes-1.c: Adjust.
	* c-c++-common/builtin-has-attribute-6.c: New test.
	* c-c++-common/builtin-has-attribute-7.c: New test.
	* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
	* c-c++-common/builtin-has-attribute-6.c: New test.
	* gcc.dg/pr25559.c: Adjust.
	* gcc.dg/attr-vector_size.c: New test.
	
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index e559d3b55d2..9ded3df278e 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -3478,19 +3478,56 @@ type_valid_for_vector_size (tree type, tree atname, tree args,
     return type;
 
   tree size = TREE_VALUE (args);
+  /* Erroneous arguments have already been diagnosed.  */
+  if (size == error_mark_node)
+    return NULL_TREE;
+
   if (size && TREE_CODE (size) != IDENTIFIER_NODE
       && TREE_CODE (size) != FUNCTION_DECL)
     size = default_conversion (size);
 
-  if (!tree_fits_uhwi_p (size))
+  if (TREE_CODE (size) != INTEGER_CST)
+    {
+      if (error_p)
+	error ("%qE attribute argument value %qE is not an integer constant",
+	       atname, size);
+      else
+	warning (OPT_Wattributes,
+		 "%qE attribute argument value %qE is not an integer constant",
+		 atname, size);
+      return NULL_TREE;
+    }
+
+  if (!TYPE_UNSIGNED (TREE_TYPE (size))
+      && tree_int_cst_sgn (size) < 0)
     {
-      /* FIXME: make the error message more informative.  */
       if (error_p)
-	warning (OPT_Wattributes, "%qE attribute ignored", atname);
+	error ("%qE attribute argument value %qE is negative",
+	       atname, size);
+      else
+	warning (OPT_Wattributes,
+		 "%qE attribute argument value %qE is negative",
+		 atname, size);
+      return NULL_TREE;
+    }
+
+  /* The attribute argument value is constrained by the maximum bit
+     alignment representable in unsigned int on the host.  */
+  unsigned HOST_WIDE_INT vecsize;
+  unsigned HOST_WIDE_INT maxsize = tree_to_uhwi (max_object_size ());
+  if (!tree_fits_uhwi_p (size)
+      || (vecsize = tree_to_uhwi (size)) > maxsize)
+    {
+      if (error_p)
+	error ("%qE attribute argument value %qE exceeds %wu",
+	       atname, size, maxsize);
+      else
+	warning (OPT_Wattributes,
+		 "%qE attribute argument value %qE exceeds %wu",
+		 atname, size, maxsize);
       return NULL_TREE;
     }
 
-  unsigned HOST_WIDE_INT vecsize = tree_to_uhwi (size);
   if (vecsize % tree_to_uhwi (TYPE_SIZE_UNIT (type)))
     {
       if (error_p)
@@ -4033,8 +4070,12 @@ validate_attribute (location_t atloc, tree oper, tree attr)
 
   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));
+  else
+    return false;
 
   /* Temporarily clear CURRENT_FUNCTION_DECL to make decl_attributes
      believe the DECL declared above is at file scope.  (See bug 87526.)  */
@@ -4043,7 +4084,7 @@ validate_attribute (location_t atloc, tree oper, tree attr)
   if (DECL_P (tmpdecl))
     {
       if (DECL_P (oper))
-	/* An alias cannot be a defintion so declare the symbol extern.  */
+	/* An alias cannot be a definition so declare the symbol extern.  */
 	DECL_EXTERNAL (tmpdecl) = true;
       /* Attribute visibility only applies to symbols visible from other
 	 translation units so make it "public."   */
@@ -4079,11 +4120,17 @@ has_attribute (location_t atloc, tree t, tree attr, tree (*convert)(tree))
       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
@@ -4134,7 +4181,8 @@ has_attribute (location_t atloc, tree t, tree attr, tree (*convert)(tree))
 	}
       else
 	{
-	  atlist = TYPE_ATTRIBUTES (TREE_TYPE (expr));
+	  type = TREE_TYPE (expr);
+	  atlist = TYPE_ATTRIBUTES (type);
 	  done = true;
 	}
 
@@ -4210,9 +4258,10 @@ has_attribute (location_t atloc, tree t, tree attr, tree (*convert)(tree))
 	      if (tree arg = TREE_VALUE (attr))
 		{
 		  arg = convert (TREE_VALUE (arg));
-		  if (expr && DECL_P (expr)
-		      && DECL_USER_ALIGN (expr)
-		      && tree_fits_uhwi_p (arg))
+		  if (!tree_fits_uhwi_p (arg))
+		    /* Invalid argument.  */;
+		  else if (expr && DECL_P (expr)
+			   && DECL_USER_ALIGN (expr))
 		    found_match = DECL_ALIGN_UNIT (expr) == tree_to_uhwi (arg);
 		  else if (type && TYPE_USER_ALIGN (type))
 		    found_match = TYPE_ALIGN_UNIT (type) == tree_to_uhwi (arg);
@@ -4249,13 +4298,7 @@ has_attribute (location_t atloc, tree t, tree attr, tree (*convert)(tree))
 	    }
 	  else if (!strcmp ("vector_size", namestr))
 	    {
-	      if (!type)
-		continue;
-
-	      /* Determine the base type from arrays, pointers, and such.
-		 Fail if the base type is not a vector.  */
-	      type = type_for_vector_size (type);
-	      if (!VECTOR_TYPE_P (type))
+	      if (!type || !VECTOR_TYPE_P (type))
 		return false;
 
 	      if (tree arg = TREE_VALUE (attr))
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index c669e49214f..6c24f2ff96a 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -8552,9 +8552,9 @@ cp_parser_has_attribute_expression (cp_parser *parser)
   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);
 
   STRIP_ANY_LOCATION_WRAPPER (oper);
 
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index 318f7e9784a..cfde248dd3d 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1247,14 +1247,18 @@ constant_alignment_word_strings (const_tree exp, HOST_WIDE_INT align)
   return align;
 }
 
-/* Default to natural alignment for vector types.  */
+/* Default to natural alignment for vector types, bounded by
+   MAX_OFILE_ALIGNMENT.  */
+
 HOST_WIDE_INT
 default_vector_alignment (const_tree type)
 {
-  HOST_WIDE_INT align = tree_to_shwi (TYPE_SIZE (type));
-  if (align > MAX_OFILE_ALIGNMENT)
-    align = MAX_OFILE_ALIGNMENT;
-  return align;
+  unsigned HOST_WIDE_INT align = MAX_OFILE_ALIGNMENT;
+  tree size = TYPE_SIZE (type);
+  if (tree_fits_uhwi_p (size))
+    align = tree_to_uhwi (size);
+
+  return align < MAX_OFILE_ALIGNMENT ? align : MAX_OFILE_ALIGNMENT;
 }
 
 /* The default implementation of
diff --git a/gcc/testsuite/c-c++-common/attributes-1.c b/gcc/testsuite/c-c++-common/attributes-1.c
index c4b232dad00..b73d31aa5e2 100644
--- a/gcc/testsuite/c-c++-common/attributes-1.c
+++ b/gcc/testsuite/c-c++-common/attributes-1.c
@@ -4,7 +4,7 @@
 void* my_calloc(unsigned, unsigned) __attribute__((alloc_size(1,bar))); /* { dg-warning ".alloc_size. attribute argument 2 is invalid" } */
 void* my_realloc(void*, unsigned) __attribute__((alloc_size(bar))); /* { dg-warning ".alloc_size. attribute argument is invalid" } */
 
-typedef char vec __attribute__((vector_size(bar))); /* { dg-warning "ignored" } */
+typedef char vec __attribute__((vector_size(bar)));
 
 void f1(char*) __attribute__((nonnull(bar))); /* { dg-warning ".nonnull. attribute argument is invalid" } */
 
@@ -14,7 +14,7 @@ void foo(int);
 void* my_calloc(unsigned, unsigned) __attribute__((alloc_size(1,foo))); /* { dg-warning ".alloc_size. attribute argument 2 has type .void\\\(int\\\)." } */
 void* my_realloc(void*, unsigned) __attribute__((alloc_size(foo))); /* { dg-warning ".alloc_size. attribute argument has type .void ?\\\(int\\\)" } */
 
-typedef char vec __attribute__((vector_size(foo))); /* { dg-warning "ignored" } */
+typedef char vec __attribute__((vector_size(foo))); /* { dg-error ".vector_size. attribute argument value .foo. is not an integer constant" } */
 
 void f1(char*) __attribute__((nonnull(foo))); /* { dg-warning ".nonnull. attribute argument has type .void ?\\\(int\\\)." } */
 void f2(char*) __attribute__((nonnull(1,foo))); /* { dg-warning ".nonnull. attribute argument 2 has type .void ?\\\(int\\\)." } */
diff --git a/gcc/testsuite/c-c++-common/builtin-has-attribute-4.c b/gcc/testsuite/c-c++-common/builtin-has-attribute-4.c
index a0681fa6980..ec3127794b5 100644
--- a/gcc/testsuite/c-c++-common/builtin-has-attribute-4.c
+++ b/gcc/testsuite/c-c++-common/builtin-has-attribute-4.c
@@ -3,7 +3,7 @@
    { dg-skip-if "No section attribute" { { hppa*-*-hpux* } && { ! lp64 } } }
    { dg-options "-Wall -ftrack-macro-expansion=0" }
    { dg-options "-Wall -Wno-narrowing -Wno-unused -ftrack-macro-expansion=0" { target c++ } }
-   { dg-additional-options "-DSKIP_ALIAS" { target *-*-darwin* } } 
+   { dg-additional-options "-DSKIP_ALIAS" { target *-*-darwin* } }
 */
 
 #define ATTR(...) __attribute__ ((__VA_ARGS__))
@@ -155,7 +155,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)), ...))
@@ -165,7 +166,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);  */
 }
@@ -218,13 +220,68 @@ void test_vector_size (void)
   A (1, iv16, vector_size (16));
   A (0, iv16, vector_size (32));
 
+  /* Verify that the attribute not detected on an array of vectors
+     but is detected on its elements.  */
+  typedef ATTR (vector_size (8)) float afv8_t[4];
+  A (0, afv8_t, vector_size);
+  A (0, afv8_t, vector_size (1));
+  A (0, afv8_t, vector_size (2));
+  A (0, afv8_t, vector_size (4));
+  A (0, afv8_t, vector_size (8));
+  A (0, afv8_t, vector_size (16));
+
+  A (1, __typeof__ ((*(afv8_t*)0)[0]), vector_size);
+  A (0, __typeof__ ((*(afv8_t*)0)[1]), vector_size (1));
+  A (0, __typeof__ ((*(afv8_t*)0)[2]), vector_size (2));
+  A (0, __typeof__ ((*(afv8_t*)0)[3]), vector_size (4));
+  A (1, __typeof__ ((*(afv8_t*)0)[0]), vector_size (8));
+  A (0, __typeof__ ((*(afv8_t*)0)[1]), vector_size (16));
+
+  A (1, __typeof__ (**(afv8_t*)0), vector_size);
+  A (0, __typeof__ (**(afv8_t*)0), vector_size (1));
+  A (0, __typeof__ (**(afv8_t*)0), vector_size (2));
+  A (0, __typeof__ (**(afv8_t*)0), vector_size (4));
+  A (1, __typeof__ (**(afv8_t*)0), vector_size (8));
+  A (0, __typeof__ (**(afv8_t*)0), vector_size (16));
+
   ATTR (vector_size (8)) float afv8[4];
-  A (1, afv8, vector_size);
+  A (0, afv8, vector_size);
   A (0, afv8, vector_size (1));
   A (0, afv8, vector_size (2));
   A (0, afv8, vector_size (4));
-  A (1, afv8, vector_size (8));
+  A (0, afv8, vector_size (8));
   A (0, afv8, vector_size (16));
+
+  A (1, afv8[0], vector_size);
+  A (0, afv8[1], vector_size (1));
+  A (0, afv8[2], vector_size (2));
+  A (0, afv8[3], vector_size (4));
+  A (1, afv8[0], vector_size (8));
+  A (0, afv8[1], vector_size (16));
+
+  A (1, *afv8, vector_size);
+  A (0, *afv8, vector_size (1));
+  A (0, *afv8, vector_size (2));
+  A (0, *afv8, vector_size (4));
+  A (1, *afv8, vector_size (8));
+  A (0, *afv8, vector_size (16));
+
+  /* sizeof (long double) is 12 on i386.  */
+  enum { VecSize = 8 * sizeof (long double) };
+  ATTR (vector_size (VecSize)) long double aldv[1][2][3];
+  A (0, aldv, vector_size);
+  A (0, aldv[0], vector_size);
+  A (0, aldv[0][0], vector_size);
+  A (1, aldv[0][0][0], vector_size);
+  A (0, aldv[0][0][1], vector_size (VecSize / 2));
+  A (1, aldv[0][0][2], vector_size (VecSize));
+
+  A (0, aldv[0][0][0][0], vector_size);
+
+  A (0, *aldv, vector_size);
+  A (0, **aldv, vector_size);
+  A (1, ***aldv, vector_size);
+  A (1, ***aldv, vector_size (VecSize));
 }
 
 
diff --git a/gcc/testsuite/c-c++-common/builtin-has-attribute-6.c b/gcc/testsuite/c-c++-common/builtin-has-attribute-6.c
new file mode 100644
index 00000000000..89cf4f23d47
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/builtin-has-attribute-6.c
@@ -0,0 +1,114 @@
+/* PR c/88383 - ICE calling _builtin_has_attribute(r, aligned(N)))
+   on an overaligned reference r
+   PR c/89288 - ICE in tree_code_size, at tree.c:865
+   { 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 doesn't confuse the attribute on
+     the pointer type with that to the pointed to type.  This
+     also exercises PR c/89288.  */
+  A (0, (Int8*)0, aligned);
+  A (0, (int*)0,  aligned);
+  A (0, (void*)0, aligned);
+  A (0, 0,        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));
+
+}
diff --git a/gcc/testsuite/c-c++-common/builtin-has-attribute-7.c b/gcc/testsuite/c-c++-common/builtin-has-attribute-7.c
new file mode 100644
index 00000000000..6ea2e9e7192
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/builtin-has-attribute-7.c
@@ -0,0 +1,396 @@
+/* Verify that __builtin_has_attribute detects attributes aligned
+   and packed in various forms of array dereferencing and indirection
+   expressions correspondingly to __alignof__.
+   { dg-do compile }
+   { dg-options "-Wall -Wno-unused -ftrack-macro-expansion=0" } */
+
+#define ATTR(...) __attribute__ ((__VA_ARGS__))
+#define ALIGN(N)  ATTR (aligned (N))
+
+#define Assert(expr) typedef int _Assert [1 - 2 * !(expr)]
+
+/* Verify that  __builtin_has_attribute (EXPR, align (ALIGN)) returns
+   the EXPECTed result.  When EXPECT is true, verify that the EXPression
+   has the expected ALIGNment.    */
+#define A3(expect, expr, align) do {					\
+    Assert (!expect || __alignof__ (expr) == align);			\
+    Assert (expect == __builtin_has_attribute (expr, aligned (align))); \
+  } while (0)
+
+#define A(expect, expr)							\
+  Assert (expect == __builtin_has_attribute (expr, aligned))		\
+
+enum { PA = __alignof__ (void*) };
+
+/* Define pointer to pointer types, with different alignments
+   at each level of indirection.  */
+typedef struct S8 { char a[8]; }   S8;
+typedef ALIGN (8)  S8              I8;
+typedef ALIGN (16) I8             *P16_I8;
+typedef            P16_I8         *P_P16_I8;
+typedef ALIGN (32) P_P16_I8       *P32_P_P16_I8;
+typedef            P32_P_P16_I8   *P_P32_P_P16_I8;
+typedef ALIGN (64) P_P32_P_P16_I8 *P64_P_P32_P_P16_I8;
+
+Assert ( 8 == __alignof__ (I8));
+Assert (16 == __alignof__ (P16_I8));
+Assert (PA == __alignof__ (P_P16_I8));
+Assert (32 == __alignof__ (P32_P_P16_I8));
+Assert (PA == __alignof__ (P_P32_P_P16_I8));
+Assert (64 == __alignof__ (P64_P_P32_P_P16_I8));
+
+
+/* Similar to the pointer of pointers above, define array of array
+   types, with different alignments at each level of indirection.  */
+typedef struct S64 { char a[64]; } S64;
+typedef ALIGN (64) S64             I64;
+typedef ALIGN (32) I64             A32_I64[3];
+typedef            A32_I64         A_A32_I64[5];
+typedef ALIGN (16) A_A32_I64       A16_A_A32_I64[7];
+typedef            A16_A_A32_I64   A_A16_A_A32_I64[11];
+typedef ALIGN (8)  A_A16_A_A32_I64 A8_A_A16_A_A32_I64[13];
+
+Assert (64 == __alignof__ (I64));
+Assert (32 == __alignof__ (A32_I64));
+/* With no explicit alignment, an array of overaligned elements
+   is considered to have the alignment of its elements.  */
+Assert (32 == __alignof__ (A_A32_I64));
+Assert (16 == __alignof__ (A16_A_A32_I64));
+Assert (16 == __alignof__ (A_A16_A_A32_I64));
+Assert ( 8 == __alignof__ (A8_A_A16_A_A32_I64));
+
+
+void test_arrays (void)
+{
+  /* Verify that the aligned attribute on each of the composite types
+     is detected corresponding to the result of __alignof__.  */
+  A (1, (*(A8_A_A16_A_A32_I64*)0));
+  A3 (1, (*(A8_A_A16_A_A32_I64*)0), 8);
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0], 8);
+  /* GCC propagates the user-align bit from element types to their
+     arrays but it doesn't propagate the attribute itself.  The built-in
+     considers both the  bit and the attribute so it succeeds below even
+     though the referenced type isn't declared with the attribute.  */
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0], 8);
+  A3 (1, (*(A8_A_A16_A_A32_I64*)0)[0], 16);
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0], 32);
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0][1], 8);
+  A3 (1, (*(A8_A_A16_A_A32_I64*)0)[0][1], 16);
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0][1], 32);
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0][1][2], 16);
+  A3 (1, (*(A8_A_A16_A_A32_I64*)0)[0][1][2], 32);
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0][1][2], 64);
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0][1][2][3], 16);
+  A3 (1, (*(A8_A_A16_A_A32_I64*)0)[0][1][2][3], 32);
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0][1][2][3], 64);
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0][1][2][3][4], 32);
+  A3 (1, (*(A8_A_A16_A_A32_I64*)0)[0][1][2][3][4], 64);
+  A3 (0, (*(A8_A_A16_A_A32_I64*)0)[0][1][2][3][4], 128);
+
+  A8_A_A16_A_A32_I64 a;
+  A3 (0, a[0], 8);
+  A3 (1, a[0], 16);
+  A3 (0, a[0], 32);
+  A3 (0, a[0][1], 8);
+  A3 (1, a[0][1], 16);
+  A3 (0, a[0][1], 32);
+  A3 (0, a[0][1][2], 16);
+  A3 (1, a[0][1][2], 32);
+  A3 (0, a[0][1][2], 64);
+  A3 (0, a[0][1][2][3], 16);
+  A3 (1, a[0][1][2][3], 32);
+  A3 (0, a[0][1][2][3], 64);
+  A3 (0, a[0][1][2][3][4], 32);
+  A3 (1, a[0][1][2][3][4], 64);
+  A3 (0, a[0][1][2][3][4], 128);
+}
+
+void test_pointers (void)
+{
+  /* Verify that the aligned attribute on each of the composite pointer
+     types is detected corresponding to the result of __alignof__.  */
+  A (1, I8);
+  A3 (0, I8, 4);
+  A3 (1, I8, 8);
+
+  A (1, P16_I8);
+  A3 (0, P16_I8, 8);
+  A3 (1, P16_I8, 16);
+
+  A (0, P_P16_I8);
+  A3 (0, P_P16_I8, 8);
+  A3 (0, P_P16_I8, 16);
+
+  A (1, P32_P_P16_I8);
+  A3 (0, P32_P_P16_I8, 8);
+  A3 (0, P32_P_P16_I8, 16);
+  A3 (1, P32_P_P16_I8, 32);
+
+  A (0, P_P32_P_P16_I8);
+
+  A (1, P64_P_P32_P_P16_I8);
+  A3 (0, P64_P_P32_P_P16_I8, 8);
+  A3 (0, P64_P_P32_P_P16_I8, 16);
+  A3 (0, P64_P_P32_P_P16_I8, 32);
+  A3 (1, P64_P_P32_P_P16_I8, 64);
+
+
+  /* Verify that the attribute on each of the composite types is detected
+     in the type of each of the indirection expressions.  */
+  A (1, *(P16_I8)0);
+  A3 (1, *(P16_I8)0, 8);
+  A3 (0, *(P16_I8)0, 16);
+
+  A (1, *(P_P16_I8)0);
+  A3 (0, *(P_P16_I8)0, 8);
+  A3 (1, *(P_P16_I8)0, 16);
+
+  A (0, *(P32_P_P16_I8)0);
+  A3 (0, *(P32_P_P16_I8)0, 8);
+  A3 (0, *(P32_P_P16_I8)0, 16);
+  A3 (0, *(P32_P_P16_I8)0, 32);
+
+  A (1, *(P_P32_P_P16_I8)0);
+  A3 (1, *(P_P32_P_P16_I8)0, 32);
+
+  A (0, *(P64_P_P32_P_P16_I8)0);
+
+  /* Verify that the attribute on each of the composite types is detected
+     in the type of each of the subscipting expressions.  */
+  A (1, ((P16_I8)0)[0]);
+  A3 (1, ((P16_I8)0)[1], 8);
+  A3 (0, ((P16_I8)0)[2], 16);
+
+  A (1, ((P_P16_I8)0)[3]);
+  A3 (0, ((P_P16_I8)0)[4], 8);
+  A3 (1, ((P_P16_I8)0)[5], 16);
+
+  A (0, ((P32_P_P16_I8)0)[6]);
+  A3 (0, ((P32_P_P16_I8)0)[7], 8);
+  A3 (0, ((P32_P_P16_I8)0)[8], 16);
+  A3 (0, ((P32_P_P16_I8)0)[9], 32);
+
+  A (1, ((P_P32_P_P16_I8)0)[10]);
+  A3 (1, ((P_P32_P_P16_I8)0)[11], 32);
+
+  A (0, ((P64_P_P32_P_P16_I8)0)[12]);
+
+
+  /* Verify that the attribute on each of the composite types is detected
+     in the type of each of the subscipting expression involving variables.  */
+
+  I8                   i8;
+  P16_I8               p16_i8 = &i8;
+  P_P16_I8             p_p16_i8 = &p16_i8;
+  P32_P_P16_I8         p32_p_p16_i8 = &p_p16_i8;
+  P_P32_P_P16_I8       p_p32_p_p16_i8 = &p32_p_p16_i8;
+  P64_P_P32_P_P16_I8   p64_p_p32_p_p16_i8 = &p_p32_p_p16_i8;
+
+  A (1, p16_i8[0]);
+  A3 (1, p16_i8[1], 8);
+  A3 (0, p16_i8[2], 16);
+
+  A (1, p_p16_i8[3]);
+  A3 (0, p_p16_i8[4], 8);
+  A3 (1, p_p16_i8[5], 16);
+
+  A (0, p32_p_p16_i8[6]);
+  A3 (0, p32_p_p16_i8[7], 8);
+  A3 (0, p32_p_p16_i8[8], 16);
+  A3 (0, p32_p_p16_i8[9], 32);
+
+  A (1, p_p32_p_p16_i8[10]);
+  A3 (1, p_p32_p_p16_i8[11], 32);
+
+
+  A (1, p_p16_i8[0][1]);
+  A3 (1, p_p16_i8[1][2], 8);
+  A3 (0, p_p16_i8[2][3], 16);
+
+
+  A (0, p64_p_p32_p_p16_i8[0]);
+
+  A (1, p64_p_p32_p_p16_i8[0][1]);
+  A3 (0, p64_p_p32_p_p16_i8[0][2], 16);
+  A3 (1, p64_p_p32_p_p16_i8[0][3], 32);
+  A3 (0, p64_p_p32_p_p16_i8[0][4], 64);
+
+  A (0, p64_p_p32_p_p16_i8[0][1][2]);
+
+  A (1, p64_p_p32_p_p16_i8[0][1][2][3]);
+  A3 (0, p64_p_p32_p_p16_i8[0][1][2][4], 8);
+  A3 (1, p64_p_p32_p_p16_i8[0][1][2][4], 16);
+  A3 (0, p64_p_p32_p_p16_i8[0][1][2][4], 32);
+
+  A (1, p64_p_p32_p_p16_i8[0][1][2][3][4]);
+  A3 (1, p64_p_p32_p_p16_i8[0][1][2][3][5], 8);
+  A3 (0, p64_p_p32_p_p16_i8[0][1][2][4][6], 16);
+
+
+  /* Same as above but using the indirection expression.  */
+  A (0, *p64_p_p32_p_p16_i8);
+
+  A (1, **p64_p_p32_p_p16_i8);
+  A3 (0, **p64_p_p32_p_p16_i8, 16);
+  A3 (1, **p64_p_p32_p_p16_i8, 32);
+  A3 (0, **p64_p_p32_p_p16_i8, 64);
+
+  A (0, ***p64_p_p32_p_p16_i8);
+
+  A (1, ****p64_p_p32_p_p16_i8);
+  A3 (0, ****p64_p_p32_p_p16_i8, 8);
+  A3 (1, ****p64_p_p32_p_p16_i8, 16);
+  A3 (0, ****p64_p_p32_p_p16_i8, 32);
+
+  A (1, *****p64_p_p32_p_p16_i8);
+  A3 (1, *****p64_p_p32_p_p16_i8, 8);
+  A3 (0, *****p64_p_p32_p_p16_i8, 16);
+}
+
+
+S8 f_S8 (void);
+I8 f_I8 (void);
+P16_I8 f_P16_I8 (void);
+P_P16_I8 f_P_P16_I8 (void);
+P32_P_P16_I8 f_P32_P_P16_I8 (void);
+P_P32_P_P16_I8 f_P_P32_P_P16_I8 (void);
+P64_P_P32_P_P16_I8 f_P64_P_P32_P_P16_I8 (void);
+
+void test_function_call (void)
+{
+  /* Verify that the aligned attribute on each of the composite pointer
+     types returned by the functions is detected corresponding to
+     the result of __alignof__.  */
+
+  A (0, f_S8 ());
+
+  A (1, f_I8 ());
+  A3 (1, f_I8 (), 8);
+  A3 (0, f_I8 (), 16);
+
+  A (1, f_P16_I8 ());
+  A3 (0, f_P16_I8 (), 8);
+  A3 (1, f_P16_I8 (), 16);
+  A3 (0, f_P16_I8 (), 32);
+
+  A (1, *f_P16_I8 ());
+  A3 (1, *f_P16_I8 (), 8);
+  A3 (0, *f_P16_I8 (), 16);
+
+  A (0, f_P_P16_I8 ());
+
+  A (1, *f_P_P16_I8 ());
+  A3 (0, *f_P_P16_I8 (), 8);
+  A3 (1, *f_P_P16_I8 (), 16);
+  A3 (0, *f_P_P16_I8 (), 32);
+
+  A (1, **f_P_P16_I8 ());
+  A3 (1, **f_P_P16_I8 (), 8);
+  A3 (0, **f_P_P16_I8 (), 16);
+  A3 (0, **f_P_P16_I8 (), 32);
+}
+
+
+void test_compound_literal (void)
+{
+  A (0, (S8){ });
+
+  A (1, (I8){ });
+  A3 (1, (I8){ }, 8);
+  A3 (0, (I8){ }, 16);
+
+  A (1, (I64){ });
+  A3 (0, (I64){ }, 8);
+  A3 (0, (I64){ }, 16);
+  A3 (0, (I64){ }, 32);
+  A3 (1, (I64){ }, 64);
+
+  A (1, (A32_I64){ 0 });
+  A3 (0, (A32_I64){ 0 }, 8);
+  A3 (0, (A32_I64){ 0 }, 16);
+  A3 (1, (A32_I64){ 0 }, 32);
+  A3 (0, (A32_I64){ 0 }, 64);
+
+  A (1, ((A32_I64){ 0 })[0]);
+  A3 (0, ((A32_I64){ 0 })[0], 8);
+  A3 (0, ((A32_I64){ 0 })[0], 16);
+  A3 (0, ((A32_I64){ 0 })[0], 32);
+  A3 (1, ((A32_I64){ 0 })[0], 64);
+}
+
+
+void test_ternary_expression (int i)
+{
+  A (0, (0 ? (S8){ } : (S8){ }));
+
+  A (1, (1 ? (I8){ } : (I8){ }));
+  A3 (1, (2 ? (I8){ } : (I8){ }), 8);
+  A3 (0, (3 ? (I8){ } : (I8){ }), 16);
+
+  A (1, (4 ? (I64){ } : (I64){ }));
+  A3 (0, (5 ? (I64){ } : (I64){ }), 8);
+  A3 (0, (6 ? (I64){ } : (I64){ }), 16);
+  A3 (0, (7 ? (I64){ } : (I64){ }), 32);
+  A3 (1, (8 ? (I64){ } : (I64){ }), 64);
+
+#if !__cplusplus
+  /* Suppress -Wc++-compat warning: converting an array compound literal
+     to a pointer is ill-formed in C++  */
+# pragma GCC diagnostic ignored "-Wc++-compat"
+
+  A (0, (9 ? (A32_I64){ } : (A32_I64){ })); 
+  A3 (0, (i ? (A32_I64){ } : (A32_I64){ }), 8);
+  A3 (0, (i++ ? (A32_I64){ } : (A32_I64){ }), 16);
+  A3 (0, (++i ? (A32_I64){ } : (A32_I64){ }), 32);
+  A3 (0, (!i ? (A32_I64){ } : (A32_I64){ }), 64);
+
+  A (1, (0 ? (A32_I64){ } : (A32_I64){ })[0]);
+  A3 (0, (1 ? (A32_I64){ } : (A32_I64){ })[1], 8);
+  A3 (0, (2 ? (A32_I64){ } : (A32_I64){ })[2], 16);
+  A3 (0, (3 ? (A32_I64){ } : (A32_I64){ })[3], 32);
+  A3 (1, (3 ? (A32_I64){ } : (A32_I64){ })[i], 64);
+#endif
+}
+
+
+void test_comma_expression (int i)
+{
+#if __cplusplus
+  /* In C++, the type of the comma expressions whose operand is an array
+     is the array itself with any attributes it was defined with.  */
+# define R 1
+#else
+  /* In C, the type of the comma expressions whose operand is an array
+     is a pointer type that does not include any attributes the array
+     was defined with.  */
+# define R 0
+/* Suppress -Wc++-compat warning: converting an array compound literal
+   to a pointer is ill-formed in C++
+   G++ accepts the conversion in unevaluated contexts without a warning.  */
+# pragma GCC diagnostic ignored "-Wc++-compat"
+#endif
+
+  A (0, (0, (S8){ }));
+
+  A (1, (0, (I8){ }));
+  A3 (1, (1, (I8){ }), 8);
+  A3 (0, (2, (I8){ }), 16);
+
+  A (1, (3, (I64){ }));
+  A3 (0, (4, (I64){ }), 8);
+  A3 (0, (5, (I64){ }), 16);
+  A3 (0, (6, (I64){ }), 32);
+  A3 (1, (7, (I64){ }), 64);
+
+  A (R, (8, (A32_I64){ }));
+  A3 (0, (9, (A32_I64){ }), 8);
+  A3 (0, ((void)0, (A32_I64){ }), 16);
+  A3 (R, ((I64){ },(A32_I64){ }), 32);
+  A3 (0, (0, (A32_I64){ }), 64);
+
+  A (1, (1, ((A32_I64){ })[0]));
+  A3 (0, (2, ((A32_I64){ })[0]), 8);
+  A3 (0, (i++, ((A32_I64){ })[0]), 16);
+  A3 (0, (++i, ((A32_I64){ })[0]), 32);
+  A3 (1, (i = 0, ((A32_I64){ })[0]), 64);
+}
diff --git a/gcc/testsuite/gcc.dg/attr-vector_size.c b/gcc/testsuite/gcc.dg/attr-vector_size.c
new file mode 100644
index 00000000000..00be26accd5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-vector_size.c
@@ -0,0 +1,69 @@
+/* PR middle-end/89797 - ICE on a vector_size (1LU << 33) int variable
+   PR c/89798 - excessive vector_size silently accepted and truncated
+   { dg-do compile { target int32plus } }
+   { dg-options "-Wall -Wno-unused" } */
+
+#define ASSERT(e)    _Static_assert (e, #e)
+#define VEC(N)       __attribute__ ((vector_size (N)))
+#define POW2(N)      (1LLU << N)
+#define CAT(a, b)    a ## b
+#define CONCAT(a, b) CAT (a, b)
+
+#define DEFVEC(storage, N)				\
+  typedef VEC (POW2 (N)) char CONCAT (Vec, N);		\
+  storage CONCAT (Vec, N) CONCAT (v, N);		\
+  ASSERT (sizeof (CONCAT (Vec, N)) == POW2 (N));	\
+  ASSERT (sizeof (CONCAT (v, N)) == POW2 (N))
+
+DEFVEC (extern, 27);
+DEFVEC (extern, 28);
+DEFVEC (extern, 29);
+DEFVEC (extern, 30);
+
+#if __SIZEOF_SIZE_T__ > 4
+
+DEFVEC (extern, 31);
+DEFVEC (extern, 32);
+DEFVEC (extern, 33);
+DEFVEC (extern, 34);
+DEFVEC (extern, 60);
+DEFVEC (extern, 61);
+DEFVEC (extern, 62);
+
+VEC (POW2 (63)) char v63;     /* { dg-error  "'vector_size' attribute argument value '9223372036854775808' exceeds 9223372036854775807" "LP64" { target lp64 } } */
+
+#else
+
+VEC (POW2 (31)) char v31;     /* { dg-error  "'vector_size' attribute argument value '2147483648' exceeds 2147483647" "ILP32" { target ilp32 } } */
+
+VEC (POW2 (32)) char v32;     /* { dg-error  "'vector_size' attribute argument value '4294967296' exceeds 2147483647" "ILP32" { target ilp32 } } */
+
+#endif
+
+void test_local_scope (void)
+{
+  DEFVEC (auto, 27);
+  DEFVEC (auto, 28);
+  DEFVEC (auto, 29);
+  DEFVEC (auto, 30);
+
+#if __SIZEOF_SIZE_T__ > 4
+
+  DEFVEC (auto, 31);
+  DEFVEC (auto, 32);
+  DEFVEC (auto, 33);
+  DEFVEC (auto, 34);
+  DEFVEC (auto, 60);
+  DEFVEC (auto, 61);
+  DEFVEC (auto, 62);
+
+  VEC (POW2 (63)) char v63;   /* { dg-error  "'vector_size' attribute argument value '9223372036854775808' exceeds 9223372036854775807" "LP64" { target lp64 } } */
+
+#else
+
+  VEC (POW2 (31)) char v31;   /* { dg-error  "'vector_size' attribute argument value '2147483648' exceeds 2147483647" "ILP32" { target ilp32 } } */
+
+  VEC (POW2 (32)) char v32;   /* { dg-error  "'vector_size' attribute argument value '4294967296' exceeds 2147483647" "ILP32" { target ilp32 } } */
+
+#endif
+}
diff --git a/gcc/testsuite/gcc.dg/pr25559.c b/gcc/testsuite/gcc.dg/pr25559.c
index 7879a1558b6..a8bce657b1c 100644
--- a/gcc/testsuite/gcc.dg/pr25559.c
+++ b/gcc/testsuite/gcc.dg/pr25559.c
@@ -2,7 +2,7 @@
 /* { dg-do compile } */
 
 #define vs(n) __attribute__((vector_size (n)))
-int vs (-1) a;			/* { dg-warning "attribute ignored" } */
+int vs (-1) a;			/* { dg-error ".vector_size. attribute argument value '-1' is negative" } */
 int vs (0) b;			/* { dg-error "zero vector size" } */
 int vs (1) c;			/* { dg-error "multiple of component size" } */
 int vs (sizeof (int) / 2) d;	/* { dg-error "multiple of component size" } */
diff --git a/gcc/tree.h b/gcc/tree.h
index 0e8e8dff876..94a810694a5 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -3741,7 +3741,7 @@ TYPE_VECTOR_SUBPARTS (const_tree node)
       return res;
     }
   else
-    return 1 << precision;
+    return (unsigned HOST_WIDE_INT)1 << precision;
 }
 
 /* Set the number of elements in VECTOR_TYPE NODE to SUBPARTS, which must

Re: [PATCH] fix ICEs in c-attribs.c (PR 88383, 89288, 89798, 89797)​

[Jeff Law]

On 4/2/19 3:19 PM, Martin Sebor wrote:
> This is yet another follow up on the discussion of the fix for
> the ICE in __builtin_has_attribute that started last December
> with PR88383.
> 
> After my last post last week I went and added more tests to make
> sure the built-in behaves as intended by comparing its result for
> non-trivial expressions with that of __alignof__.  The test that
> does this is the new builtin-has-attribute-7.c.
> 
> The test exposed one problem in the handling of attribute vector_size
> by the built-in (I mentioned that in my last post).  It also exposed
> a couple of bugs in the attribute handler itself.  I fixed both of
> these in the attached patch.
> 
> This latest revision of the patch resolves the following bugs:
> 
>   PR 88383 - ICE calling __builtin_has_attribute on a reference
>   PR 89288 - ICE in tree_code_size, at tree.c:865
>   PR 89798 - excessive vector_size silently accepted and truncated
>   PR 89797 - ICE on a vector_size (1LU << 33) int variable
> 
> Bootstrapped on x86_64-linux.  The tests are still running but
> assuming they pass, is this last revision good to commit?
> 
> Martin
> 
> A link to my last comment in the archive:
>   https://gcc.gnu.org/ml/gcc-patches/2019-03/msg01096.html
> 
> gcc-88383.diff
> 
> PR c/88383 - ICE calling __builtin_has_attribute on a reference
> PR c/89288 - ICE in tree_code_size, at tree.c:865
> PR c/89798 - excessive vector_size silently accepted and truncated
> PR c/89797 - ICE on a vector_size (1LU << 33) int variable
> 
> gcc/ChangeLog:
> 
> 	PR c/89797
> 	* targhooks.c (default_vector_alignment): Avoid assuming
> 	argument fits in SHWI.
> 	* tree.h (TYPE_VECTOR_SUBPARTS): Avoid sign overflow in
> 	a shift expression.
> 
> gcc/c-family/ChangeLog:
> 
> 	PR c/88383
> 	PR c/89288
> 	PR c/89798
> 	PR c/89797
> 	* c-attribs.c (type_valid_for_vector_size): Detect excessively
> 	large sizes.
> 	(validate_attribute): Handle DECLs and expressions.
> 	(has_attribute): Handle types referenced by expressions.
> 	Avoid considering array attributes in ARRAY_REF expressions .
> 
> gcc/cp/ChangeLog:
> 
> 	PR c/88383
> 	PR c/89288
> 	* parser.c (cp_parser_has_attribute_expression): Handle assignment
> 	expressions.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c/88383
> 	PR c/89288
> 	PR c/89798
> 	PR c/89797
> 	* c-c++-common/attributes-1.c: Adjust.
> 	* c-c++-common/builtin-has-attribute-6.c: New test.
> 	* c-c++-common/builtin-has-attribute-7.c: New test.
> 	* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
> 	* c-c++-common/builtin-has-attribute-6.c: New test.
> 	* gcc.dg/pr25559.c: Adjust.
> 	* gcc.dg/attr-vector_size.c: New test.
Per our offline discussion, I think this is OK with some documentation
updates to clarify expression handling.

In particular can you adjust the docs to more clearly indicate that for
an expression, we look at the attributes of the type of the expression.

With a suitable doc change of that nature, this is OK.

jeff

Re: [PATCH] fix ICEs in c-attribs.c (PR 88383, 89288, 89798, 89797)​

[Jakub Jelinek]

On Fri, Apr 12, 2019 at 10:45:25AM -0600, Jeff Law wrote:
> > gcc/ChangeLog:
> > 
> > 	PR c/89797
> > 	* targhooks.c (default_vector_alignment): Avoid assuming
> > 	argument fits in SHWI.
> > 	* tree.h (TYPE_VECTOR_SUBPARTS): Avoid sign overflow in
> > 	a shift expression.
> > 
> > gcc/c-family/ChangeLog:
> > 
> > 	PR c/88383
> > 	PR c/89288
> > 	PR c/89798
> > 	PR c/89797
> > 	* c-attribs.c (type_valid_for_vector_size): Detect excessively
> > 	large sizes.
> > 	(validate_attribute): Handle DECLs and expressions.
> > 	(has_attribute): Handle types referenced by expressions.
> > 	Avoid considering array attributes in ARRAY_REF expressions .
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	PR c/88383
> > 	PR c/89288
> > 	* parser.c (cp_parser_has_attribute_expression): Handle assignment
> > 	expressions.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	PR c/88383
> > 	PR c/89288
> > 	PR c/89798
> > 	PR c/89797
> > 	* c-c++-common/attributes-1.c: Adjust.
> > 	* c-c++-common/builtin-has-attribute-6.c: New test.
> > 	* c-c++-common/builtin-has-attribute-7.c: New test.
> > 	* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
> > 	* c-c++-common/builtin-has-attribute-6.c: New test.

New test twice for the same test?

> > 	* gcc.dg/pr25559.c: Adjust.
> > 	* gcc.dg/attr-vector_size.c: New test.
> Per our offline discussion, I think this is OK with some documentation
> updates to clarify expression handling.
> 
> In particular can you adjust the docs to more clearly indicate that for
> an expression, we look at the attributes of the type of the expression.
> 
> With a suitable doc change of that nature, this is OK.

Has the patch been tested at all?
I see many new FAILs after the patch:
make check-gcc check-c++-all RUNTESTFLAGS="--target_board=unix\{-m32,-m64\} dg.exp='attributes-1.c builtin-has-attribute-*.c pr71574.c attr-vector_size.c pr25559.c'"
FAIL: gcc.dg/attr-vector_size.c LP64 (test for errors, line 33)
FAIL: gcc.dg/attr-vector_size.c LP64 (test for errors, line 60)
FAIL: gcc.dg/attr-vector_size.c (test for excess errors)
FAIL: gcc.dg/pr25559.c  (test for errors, line 5)
FAIL: gcc.dg/pr25559.c (test for excess errors)
FAIL: c-c++-common/attributes-1.c  -Wc++-compat   (test for errors, line 17)
FAIL: c-c++-common/attributes-1.c  -Wc++-compat  (test for excess errors)
FAIL: c-c++-common/builtin-has-attribute-4.c  -Wc++-compat  (test for excess errors)
FAIL: c-c++-common/builtin-has-attribute-6.c  -Wc++-compat  (internal compiler error)
FAIL: c-c++-common/builtin-has-attribute-6.c  -Wc++-compat  (test for excess errors)
FAIL: c-c++-common/builtin-has-attribute-7.c  -Wc++-compat  (internal compiler error)
FAIL: c-c++-common/builtin-has-attribute-7.c  -Wc++-compat  (test for excess errors)
FAIL: c-c++-common/pr71574.c  -Wc++-compat   (test for errors, line 14)
FAIL: c-c++-common/pr71574.c  -Wc++-compat  (test for excess errors)
for -m64, 
FAIL: gcc.dg/attr-vector_size.c (internal compiler error)
FAIL: gcc.dg/attr-vector_size.c ILP32 (test for errors, line 37)
FAIL: gcc.dg/attr-vector_size.c ILP32 (test for errors, line 39)
FAIL: gcc.dg/attr-vector_size.c ILP32 (test for errors, line 64)
FAIL: gcc.dg/attr-vector_size.c ILP32 (test for errors, line 66)
FAIL: gcc.dg/attr-vector_size.c (test for excess errors)
FAIL: gcc.dg/pr25559.c  (test for errors, line 5)
FAIL: gcc.dg/pr25559.c (test for excess errors)
FAIL: c-c++-common/attributes-1.c  -Wc++-compat   (test for errors, line 17)
FAIL: c-c++-common/attributes-1.c  -Wc++-compat  (test for excess errors)
FAIL: c-c++-common/builtin-has-attribute-4.c  -Wc++-compat  (test for excess errors)
FAIL: c-c++-common/builtin-has-attribute-6.c  -Wc++-compat  (internal compiler error)
FAIL: c-c++-common/builtin-has-attribute-6.c  -Wc++-compat  (test for excess errors)
FAIL: c-c++-common/builtin-has-attribute-7.c  -Wc++-compat  (internal compiler error)
FAIL: c-c++-common/builtin-has-attribute-7.c  -Wc++-compat  (test for excess errors)
FAIL: c-c++-common/pr71574.c  -Wc++-compat   (test for errors, line 14)
FAIL: c-c++-common/pr71574.c  -Wc++-compat  (test for excess errors)
for -m32, and quite similar set for C++.

	Jakub

Re: [PATCH] fix ICEs in c-attribs.c (PR 88383, 89288, 89798, 89797)​

[Jakub Jelinek]

On Fri, Apr 12, 2019 at 11:42:59PM +0200, Jakub Jelinek wrote:
> > With a suitable doc change of that nature, this is OK.
> 
> Has the patch been tested at all?
> I see many new FAILs after the patch:
> make check-gcc check-c++-all RUNTESTFLAGS="--target_board=unix\{-m32,-m64\} dg.exp='attributes-1.c builtin-has-attribute-*.c pr71574.c attr-vector_size.c pr25559.c'"
> FAIL: gcc.dg/attr-vector_size.c LP64 (test for errors, line 33)
> FAIL: gcc.dg/attr-vector_size.c LP64 (test for errors, line 60)
> FAIL: gcc.dg/attr-vector_size.c (test for excess errors)
> FAIL: gcc.dg/pr25559.c  (test for errors, line 5)
> FAIL: gcc.dg/pr25559.c (test for excess errors)
> FAIL: c-c++-common/attributes-1.c  -Wc++-compat   (test for errors, line 17)
> FAIL: c-c++-common/attributes-1.c  -Wc++-compat  (test for excess errors)
> FAIL: c-c++-common/builtin-has-attribute-4.c  -Wc++-compat  (test for excess errors)
> FAIL: c-c++-common/builtin-has-attribute-6.c  -Wc++-compat  (internal compiler error)
> FAIL: c-c++-common/builtin-has-attribute-6.c  -Wc++-compat  (test for excess errors)
> FAIL: c-c++-common/builtin-has-attribute-7.c  -Wc++-compat  (internal compiler error)
> FAIL: c-c++-common/builtin-has-attribute-7.c  -Wc++-compat  (test for excess errors)
> FAIL: c-c++-common/pr71574.c  -Wc++-compat   (test for errors, line 14)
> FAIL: c-c++-common/pr71574.c  -Wc++-compat  (test for excess errors)
> for -m64, 
> FAIL: gcc.dg/attr-vector_size.c (internal compiler error)
> FAIL: gcc.dg/attr-vector_size.c ILP32 (test for errors, line 37)
> FAIL: gcc.dg/attr-vector_size.c ILP32 (test for errors, line 39)
> FAIL: gcc.dg/attr-vector_size.c ILP32 (test for errors, line 64)
> FAIL: gcc.dg/attr-vector_size.c ILP32 (test for errors, line 66)
> FAIL: gcc.dg/attr-vector_size.c (test for excess errors)
> FAIL: gcc.dg/pr25559.c  (test for errors, line 5)
> FAIL: gcc.dg/pr25559.c (test for excess errors)
> FAIL: c-c++-common/attributes-1.c  -Wc++-compat   (test for errors, line 17)
> FAIL: c-c++-common/attributes-1.c  -Wc++-compat  (test for excess errors)
> FAIL: c-c++-common/builtin-has-attribute-4.c  -Wc++-compat  (test for excess errors)
> FAIL: c-c++-common/builtin-has-attribute-6.c  -Wc++-compat  (internal compiler error)
> FAIL: c-c++-common/builtin-has-attribute-6.c  -Wc++-compat  (test for excess errors)
> FAIL: c-c++-common/builtin-has-attribute-7.c  -Wc++-compat  (internal compiler error)
> FAIL: c-c++-common/builtin-has-attribute-7.c  -Wc++-compat  (test for excess errors)
> FAIL: c-c++-common/pr71574.c  -Wc++-compat   (test for errors, line 14)
> FAIL: c-c++-common/pr71574.c  -Wc++-compat  (test for excess errors)
> for -m32, and quite similar set for C++.

And
https://gcc.gnu.org/ml/gcc-testresults/2019-04/msg01416.html
confirms that too.

	Jakub

Re: [PATCH] fix ICEs in c-attribs.c (PR 88383, 89288, 89798, 89797)​

[Martin Sebor]

On 4/12/19 3:42 PM, Jakub Jelinek wrote:
> On Fri, Apr 12, 2019 at 10:45:25AM -0600, Jeff Law wrote:
>>> gcc/ChangeLog:
>>>
>>> 	PR c/89797
>>> 	* targhooks.c (default_vector_alignment): Avoid assuming
>>> 	argument fits in SHWI.
>>> 	* tree.h (TYPE_VECTOR_SUBPARTS): Avoid sign overflow in
>>> 	a shift expression.
>>>
>>> gcc/c-family/ChangeLog:
>>>
>>> 	PR c/88383
>>> 	PR c/89288
>>> 	PR c/89798
>>> 	PR c/89797
>>> 	* c-attribs.c (type_valid_for_vector_size): Detect excessively
>>> 	large sizes.
...
> 
> Has the patch been tested at all?

A few times.  The c-attribs.c change above didn't make it into
the commit.

Martin

Re: [PATCH] fix ICEs in c-attribs.c (PR 88383, 89288, 89798, 89797)​

[Christophe Lyon]

On Sat, 13 Apr 2019 at 00:38, Martin Sebor <msebor@gmail.com> wrote:
>
> On 4/12/19 3:42 PM, Jakub Jelinek wrote:
> > On Fri, Apr 12, 2019 at 10:45:25AM -0600, Jeff Law wrote:
> >>> gcc/ChangeLog:
> >>>
> >>>     PR c/89797
> >>>     * targhooks.c (default_vector_alignment): Avoid assuming
> >>>     argument fits in SHWI.
> >>>     * tree.h (TYPE_VECTOR_SUBPARTS): Avoid sign overflow in
> >>>     a shift expression.
> >>>
> >>> gcc/c-family/ChangeLog:
> >>>
> >>>     PR c/88383
> >>>     PR c/89288
> >>>     PR c/89798
> >>>     PR c/89797
> >>>     * c-attribs.c (type_valid_for_vector_size): Detect excessively
> >>>     large sizes.
> ...
> >
> > Has the patch been tested at all?
>
> A few times.  The c-attribs.c change above didn't make it into
> the commit.

Hi,
Even with r270331, I'm still seeing the ICE on aarch64 (actually with
trunk @r270370)

Is there still some commit missing?

Thanks,

Christophe

> Martin

Re: [PATCH] fix ICEs in c-attribs.c (PR 88383, 89288, 89798, 89797)​

[Jeff Law]

On 4/15/19 7:12 AM, Christophe Lyon wrote:
> On Sat, 13 Apr 2019 at 00:38, Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 4/12/19 3:42 PM, Jakub Jelinek wrote:
>>> On Fri, Apr 12, 2019 at 10:45:25AM -0600, Jeff Law wrote:
>>>>> gcc/ChangeLog:
>>>>>
>>>>>     PR c/89797
>>>>>     * targhooks.c (default_vector_alignment): Avoid assuming
>>>>>     argument fits in SHWI.
>>>>>     * tree.h (TYPE_VECTOR_SUBPARTS): Avoid sign overflow in
>>>>>     a shift expression.
>>>>>
>>>>> gcc/c-family/ChangeLog:
>>>>>
>>>>>     PR c/88383
>>>>>     PR c/89288
>>>>>     PR c/89798
>>>>>     PR c/89797
>>>>>     * c-attribs.c (type_valid_for_vector_size): Detect excessively
>>>>>     large sizes.
>> ...
>>>
>>> Has the patch been tested at all?
>>
>> A few times.  The c-attribs.c change above didn't make it into
>> the commit.
> 
> Hi,
> Even with r270331, I'm still seeing the ICE on aarch64 (actually with
> trunk @r270370)
> 
> Is there still some commit missing?
> 
Or perhaps something else broken.  My tester flagged these are aarch64


> New tests that FAIL (4 tests):
> 
> gcc.dg/attr-vector_size.c (internal compiler error)
> gcc.dg/attr-vector_size.c (test for excess errors)
> gcc.dg/attr-vector_size.c LP64 (test for errors, line 33)
> gcc.dg/attr-vector_size.c LP64 (test for errors, line 60)

The rest of the tests passed.  It could well be something different
about the aarch64 port.  Seems like a bit of debugging is advisable.

jeff

Re: [PATCH] fix ICEs in c-attribs.c (PR 88383, 89288, 89798, 89797)​

[Martin Sebor]

On 4/15/19 7:12 AM, Christophe Lyon wrote:
> On Sat, 13 Apr 2019 at 00:38, Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 4/12/19 3:42 PM, Jakub Jelinek wrote:
>>> On Fri, Apr 12, 2019 at 10:45:25AM -0600, Jeff Law wrote:
>>>>> gcc/ChangeLog:
>>>>>
>>>>>      PR c/89797
>>>>>      * targhooks.c (default_vector_alignment): Avoid assuming
>>>>>      argument fits in SHWI.
>>>>>      * tree.h (TYPE_VECTOR_SUBPARTS): Avoid sign overflow in
>>>>>      a shift expression.
>>>>>
>>>>> gcc/c-family/ChangeLog:
>>>>>
>>>>>      PR c/88383
>>>>>      PR c/89288
>>>>>      PR c/89798
>>>>>      PR c/89797
>>>>>      * c-attribs.c (type_valid_for_vector_size): Detect excessively
>>>>>      large sizes.
>> ...
>>>
>>> Has the patch been tested at all?
>>
>> A few times.  The c-attribs.c change above didn't make it into
>> the commit.
> 
> Hi,
> Even with r270331, I'm still seeing the ICE on aarch64 (actually with
> trunk @r270370)
> 
> Is there still some commit missing?

I only fixed the TYPE_VECTOR_SUBPARTS bug behind the ICE in bug
89797 for targets where NUM_POLY_INT_COEFFS == 1, and only tested
it on x86_64.

The block of code that's compiled when NUM_POLY_INT_COEFFS == 1
is also buggy but from what I can tell in more ways than one.

   inline poly_uint64
   TYPE_VECTOR_SUBPARTS (const_tree node)
   {
     STATIC_ASSERT (NUM_POLY_INT_COEFFS <= 2);
     unsigned int precision = VECTOR_TYPE_CHECK 
(node)->type_common.precision;
     if (NUM_POLY_INT_COEFFS == 2)
       {
         poly_uint64 res = 0;
         res.coeffs[0] = 1 << (precision & 0xff);
         if (precision & 0x100)
   	  res.coeffs[1] = 1 << (precision & 0xff);
         return res;
       }
     else
       return (unsigned HOST_WIDE_INT)1 << precision;
   }

Shifting 1 to the left by more than 30 bits is undefined (that
was also the bug I fixed in the other branch).  In addition,
setting res.coeffs[1] to the same value as res.coeffs[0] doesn't
seem right either, but since precision should be less than 256
it may not actually matter.  In any case, I wasn't sure and
since I don't have easy access to speedy aarch64 hardware I
wasn't brave enough to mess with it at this stage.

Then there's another bug in aarch64_simd_vector_alignment
similar to the one the patch fixed in default_vector_alignment
where the code assumes that the vector alignment fits in SHWI.

The otherwise untested patch below fixes the ICE for aarch64
but not the other (suspected) bug.

Index: gcc/tree.h
===================================================================
--- gcc/tree.h	(revision 270402)
+++ gcc/tree.h	(working copy)
@@ -3735,9 +3735,9 @@ TYPE_VECTOR_SUBPARTS (const_tree node)
    if (NUM_POLY_INT_COEFFS == 2)
      {
        poly_uint64 res = 0;
-      res.coeffs[0] = 1 << (precision & 0xff);
+      res.coeffs[0] = (unsigned HOST_WIDE_INT)1 << (precision & 0xff);
        if (precision & 0x100)
-	res.coeffs[1] = 1 << (precision & 0xff);
+	res.coeffs[1] = (unsigned HOST_WIDE_INT)1 << (precision & 0xff);
        return res;
      }
    else
Index: gcc/config/aarch64/aarch64.c
===================================================================
--- gcc/config/aarch64/aarch64.c	(revision 270402)
+++ gcc/config/aarch64/aarch64.c	(working copy)
@@ -14924,7 +14924,10 @@ aarch64_simd_vector_alignment (const_tree type)
         be set for non-predicate vectors of booleans.  Modes are the most
         direct way we have of identifying real SVE predicate types.  */
      return GET_MODE_CLASS (TYPE_MODE (type)) == MODE_VECTOR_BOOL ? 16 
: 128;
-  HOST_WIDE_INT align = tree_to_shwi (TYPE_SIZE (type));
+  tree size = TYPE_SIZE (type);
+  unsigned HOST_WIDE_INT align = 128;
+  if (tree_fits_uhwi_p (size))
+    align = tree_to_uhwi (TYPE_SIZE (type));
    return MIN (align, 128);
  }

I'm guessing that the full fix for TYPE_VECTOR_SUBPARTS might look
something like this, but that's just a wild guess.

===================================================================
--- gcc/tree.h	(revision 270402)
+++ gcc/tree.h	(working copy)
@@ -3735,9 +3735,9 @@ TYPE_VECTOR_SUBPARTS (const_tree node)
    if (NUM_POLY_INT_COEFFS == 2)
      {
        poly_uint64 res = 0;
-      res.coeffs[0] = 1 << (precision & 0xff);
+      res.coeffs[0] = (unsigned HOST_WIDE_INT)1 << (precision & 0xff);
        if (precision & 0x100)
-	res.coeffs[1] = 1 << (precision & 0xff);
+	res.coeffs[1] = (unsigned HOST_WIDE_INT)1 << ((precision & 0x100) >> 16);
        return res;
      }
    else

Martin

Re: [PATCH] fix ICEs in c-attribs.c (PR 88383, 89288, 89798, 89797)​

[Jakub Jelinek]

On Tue, Apr 16, 2019 at 08:40:29PM -0600, Martin Sebor wrote:
> --- gcc/tree.h	(revision 270402)
> +++ gcc/tree.h	(working copy)
> @@ -3735,9 +3735,9 @@ TYPE_VECTOR_SUBPARTS (const_tree node)
>    if (NUM_POLY_INT_COEFFS == 2)
>      {
>        poly_uint64 res = 0;
> -      res.coeffs[0] = 1 << (precision & 0xff);
> +      res.coeffs[0] = (unsigned HOST_WIDE_INT)1 << (precision & 0xff);
>        if (precision & 0x100)
> -	res.coeffs[1] = 1 << (precision & 0xff);
> +	res.coeffs[1] = (unsigned HOST_WIDE_INT)1 << (precision & 0xff);

Instead of (unsigned HOST_WIDE_INT)1 one should use HOST_WIDE_INT_1U
macro.

	Jakub

Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)

[Jeff Law]

On 3/21/19 3:59 PM, Martin Sebor wrote:
> On 3/19/19 9:33 PM, Jeff Law wrote:
>> On 3/19/19 8:22 PM, Joseph Myers wrote:
>>> On Tue, 19 Mar 2019, Jeff Law wrote:
>>>
>>>> I'll note that our documentation clearly states that attributes can be
>>>> applied to functions, variables, labels, enums, statements and types.
>>>
>>> A key thing here is that they can be applied to fields - that is,
>>> they can
>>> be properties of a FIELD_DECL.  Referring to a field either requires an
>>> expression referring to that field, or some other custom syntax that
>>> uses
>>> both a type name and a field name (like in __builtin_offsetof).
>>>
>> Thanks for chiming in, your opinions on this kind of thing are greatly
>> appreciated.
>>
>> Understood WRT applying to fields, and I think that's consistent with
>> some of what Jakub has expressed elsewhere -- specifically that we
>> should consider allowing COMPONENT_REFs as the exception, returning the
>> attributes of the associated FIELD_DECL in that case.
>>
>> Is there a need to call out BIT_FIELD_REFs where we can't actually get
>> to the underlying DECL?  And if so, how do we do that in the end user
>> documentation that is clear and consistent?
> 
> Is it possible to see a BIT_FIELD_REF here?  I couldn't find a way.
Unsure.  It was a generic concern -- I don't have a motivating example.
 From very light reading in the front-ends, you're more likely to get
one from the C++ front-end rather the C front-end.  It may also be able
to get a BIT_FIELD_REF from some OMP constructs.  In general it looks
like the front-ends aren't generating them often, preferring to stick
with COMPONENT_REF instead.




> 
>>
>> One of the big worries I've got here is that documenting when an
>> expression as an argument to __builtin_has_attribute (or any attribute
>> query) can be expected to work.  It's always easier on our end users to
>> loosen semantics of extensions over time than it is to tighten them.
> 
> I wonder if a part of the issue isn't due to a mismatch between
> the C terminology and what GCC uses internally.  Every argument
> to the built-in that's not a type (and every argument to attribute
> copy which doesn't accept types) must be a C expression:
My hesitation has been based more on the core concept that we don't
attach attributes to expressions, only types and decls.  Thus asking if
an expression has any given attribute doesn't make much sense at first
glance.

In the copy attribute case the docs were pretty clear when and how it
applied to expressions -- specifically stating that we're going to look
at the underlying type and pull the attributes from there.

Now that in turn raises its own set of issues.  If the front-ends were
to change the type of an expression (and they have in the past,
particularly for COMPONENT_REF/INDIRECT_REF to avoid subsequent
nop-casts), then we have to worry about the inconsistencies in behavior
that's going to expose to the user.  Another case where that could
potentially happen would be LTO.  I believe LTO will canonicalize/merge
types to some degree and I don't offhand know the rules there and
inconsistencies could be exposed to the user.

Given that I think the front-end code that changed types of
COMPONENT_REF/INDIRECT_REF to avoid subsequent nop-conversions is no
longer in the tree and that (in theory) type merging in LTO should be
considering attributes I went ahead and pushed those concerns aside for
the copy attribute patch.

I'll be doing the same when re-re-re-reviewing the
__builtin_has_attribute patch :-)


jeff