Message ID | c6ea0ad5-8680-79e0-049c-85ec297555df@gmail.com |
---|---|
State | New |
Headers | show |
Series | fix ICE in __builtin_has_attribute (PR 88383 and 89288) | expand |
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
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)); + +}