fix ICE in __builtin_has_attribute (PR 88383 and 89288)

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

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

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

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

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

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

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

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

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

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

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

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

>> 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.

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

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

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).

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

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

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

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

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

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

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

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