Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
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
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
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
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On 3/21/19 4:25 PM, Jakub Jelinek wrote: On Thu, Mar 21, 2019 at 04:19:37PM -0600, Martin Sebor wrote: Say that the argument is either a type or an expression that is either an identifier (for C++ id-expression) to cover 1) and a postfix expression with . or -> operator (to cover COMPONENT_REF)? That doesn't look easy to understand. Why? Those users that don't read corresponding language standards will just see the compiler diagnosing any uses but those 3 kinds and can then just read the documentation which will show in examples what is accepted and what it will do. Because to most users it suggests that . and -> are the only operators that are accepted in the expression and so something like p[0].a is not a valid argument. Those who don't read the manual will be puzzled when, after having had p[0].a accepted, p->a[0] will trigger an error. We do not want to allow INDIRECT_REF, ARRAY_REF, etc. Why not? What exactly is the concern? Because only the . and -> operators are needed to get at the non-static member declaration. INDIRECT_REF or ARRAY_REF aren't useful for that, and then it raises the question what exactly is supposed to be the behavior when you use *expr or expr[0] or expr->field[0]. Those expression don't have any decl, so we'd be back at your suggestion to pretty randomly sometimes return DECL_ATTRIBUTES, sometimes TYPE_ATTRIBUTES, sometimes both. That is just a bad design. If people want type attributes, they should use __typeof or decltype or just type itself in the argument. I never suggested to "pretty randomly sometimes return DECL_ATTRIBUTES, sometimes TYPE_ATTRIBUTES, or sometimes both." That's a silly mischaracterization, just as it would be to say the same thing about __alignof__, and it works much the same way. It operates on expressions and returns the alignment either explicitly speciified for the variable (or function) or that specified for its type. I modeled the built-in on __alignof__. There may bugs where it behaves differently, or subtle deviations where I chose a different approach, but it's certainly not random. I'd like to fix the bugs, and I'm open to reconsidering the deviations, but using either as a justification for rejecting those kinds of expressions would result in hard-to-use API. And _that_ would be bad design. Martin
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On Thu, Mar 21, 2019 at 04:19:37PM -0600, Martin Sebor wrote: > > Say that the argument is either a type or an expression that is > > either an identifier (for C++ id-expression) to cover 1) and > > a postfix expression with . or -> operator (to cover COMPONENT_REF)? > > That doesn't look easy to understand. Why? Those users that don't read corresponding language standards will just see the compiler diagnosing any uses but those 3 kinds and can then just read the documentation which will show in examples what is accepted and what it will do. > > We do not want to allow INDIRECT_REF, ARRAY_REF, etc. > > Why not? What exactly is the concern? Because only the . and -> operators are needed to get at the non-static member declaration. INDIRECT_REF or ARRAY_REF aren't useful for that, and then it raises the question what exactly is supposed to be the behavior when you use *expr or expr[0] or expr->field[0]. Those expression don't have any decl, so we'd be back at your suggestion to pretty randomly sometimes return DECL_ATTRIBUTES, sometimes TYPE_ATTRIBUTES, sometimes both. That is just a bad design. If people want type attributes, they should use __typeof or decltype or just type itself in the argument. Jakub
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On 3/21/19 4:13 PM, Jakub Jelinek wrote: On Thu, Mar 21, 2019 at 03:59:55PM -0600, Martin Sebor wrote: 1) either an identifier naming a function or variable, or 2) some other expression like a member reference via . or ->, an array subscript, or the indirection expression *. But GCC distinguishes three kinds of arguments: 1) a DECL, 2) some sort of a reference like ARRAY_REF, COMPONENT_REF or INDIRECT_REF 3) an expression that satisfies the EXPR_P() predicate (e.g., (struct S*)0, or (struct S){ 1 }) Jeff, you seem to want the built-in to accept just (1) on the GCC list above and reject (3) (and seem to be waffling on (2)). How would such an argument be described in a way that users unfamiliar with GCC internals could easily understand? Say that the argument is either a type or an expression that is either an identifier (for C++ id-expression) to cover 1) and a postfix expression with . or -> operator (to cover COMPONENT_REF)? That doesn't look easy to understand. We do not want to allow INDIRECT_REF, ARRAY_REF, etc. Why not? What exactly is the concern? Martin
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On Thu, Mar 21, 2019 at 03:59:55PM -0600, Martin Sebor wrote: > 1) either an identifier naming a function or variable, or > 2) some other expression like a member reference via . or ->, >an array subscript, or the indirection expression *. > > But GCC distinguishes three kinds of arguments: > > 1) a DECL, > 2) some sort of a reference like ARRAY_REF, COMPONENT_REF or >INDIRECT_REF > 3) an expression that satisfies the EXPR_P() predicate (e.g., >(struct S*)0, or (struct S){ 1 }) > > Jeff, you seem to want the built-in to accept just (1) on the GCC > list above and reject (3) (and seem to be waffling on (2)). > > How would such an argument be described in a way that users > unfamiliar with GCC internals could easily understand? Say that the argument is either a type or an expression that is either an identifier (for C++ id-expression) to cover 1) and a postfix expression with . or -> operator (to cover COMPONENT_REF)? We do not want to allow INDIRECT_REF, ARRAY_REF, etc. Jakub
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On 3/19/19 9:33 PM, Jeff Law wrote: On 3/19/19 8:22 PM, Joseph Myers wrote: On Tue, 19 Mar 2019, Jeff Law wrote: I'll note that our documentation clearly states that attributes can be applied to functions, variables, labels, enums, statements and types. A key thing here is that they can be applied to fields - that is, they can be properties of a FIELD_DECL. Referring to a field either requires an expression referring to that field, or some other custom syntax that uses both a type name and a field name (like in __builtin_offsetof). Thanks for chiming in, your opinions on this kind of thing are greatly appreciated. Understood WRT applying to fields, and I think that's consistent with some of what Jakub has expressed elsewhere -- specifically that we should consider allowing COMPONENT_REFs as the exception, returning the attributes of the associated FIELD_DECL in that case. Is there a need to call out BIT_FIELD_REFs where we can't actually get to the underlying DECL? And if so, how do we do that in the end user documentation that is clear and consistent? Is it possible to see a BIT_FIELD_REF here? I couldn't find a way. One of the big worries I've got here is that documenting when an expression as an argument to __builtin_has_attribute (or any attribute query) can be expected to work. It's always easier on our end users to loosen semantics of extensions over time than it is to tighten them. I wonder if a part of the issue isn't due to a mismatch between the C terminology and what GCC uses internally. Every argument to the built-in that's not a type (and every argument to attribute copy which doesn't accept types) must be a C expression: 1) either an identifier naming a function or variable, or 2) some other expression like a member reference via . or ->, an array subscript, or the indirection expression *. But GCC distinguishes three kinds of arguments: 1) a DECL, 2) some sort of a reference like ARRAY_REF, COMPONENT_REF or INDIRECT_REF 3) an expression that satisfies the EXPR_P() predicate (e.g., (struct S*)0, or (struct S){ 1 }) Jeff, you seem to want the built-in to accept just (1) on the GCC list above and reject (3) (and seem to be waffling on (2)). How would such an argument be described in a way that users unfamiliar with GCC internals could easily understand? All sorts of expressions can be used to refer to fields. Given the type definition 'struct A { int b[2]; };' besides FUNCTION_DECL, PARM_DECL, and VAR_DECL we might expect to commonly see arguments like: COMPONENT_REF: ((struct A*)0)->b (*((struct A*)0)).b ((struct A*)0)[0].b INDIRECT_REF: *((struct A*)0)[0].b ARRAY_REF: ((struct A*)0)[0].b[0] To users, they're all just expressions. Fields aside, the expressions below seem quite plausible to me too, especially when the built-in argument is the result of macro expansion: CALL_EXPR: foo () COMPOUND_LITERAL_EXPR: (struct A){ ... } COND_EXPR: (0 ? a0 : a1) COMPOUND_EXPR: ((void)0, a) NON_LVALUE_EXPR: (struct A)a etc. In these cases the built-in could be passed the result of __typeof__ instead, but if the built-in itself is part of a macro that can be invoked either with one of the arguments on the _REF list or with one of these expressions, it would only work as expected for half of them. Using __typeof__ doesn't work for the copy attribute because it only takes expressions as arguments and not types. Martin
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On 3/19/19 8:22 PM, Joseph Myers wrote: > On Tue, 19 Mar 2019, Jeff Law wrote: > >> I'll note that our documentation clearly states that attributes can be >> applied to functions, variables, labels, enums, statements and types. > > A key thing here is that they can be applied to fields - that is, they can > be properties of a FIELD_DECL. Referring to a field either requires an > expression referring to that field, or some other custom syntax that uses > both a type name and a field name (like in __builtin_offsetof). > Thanks for chiming in, your opinions on this kind of thing are greatly appreciated. Understood WRT applying to fields, and I think that's consistent with some of what Jakub has expressed elsewhere -- specifically that we should consider allowing COMPONENT_REFs as the exception, returning the attributes of the associated FIELD_DECL in that case. Is there a need to call out BIT_FIELD_REFs where we can't actually get to the underlying DECL? And if so, how do we do that in the end user documentation that is clear and consistent? One of the big worries I've got here is that documenting when an expression as an argument to __builtin_has_attribute (or any attribute query) can be expected to work. It's always easier on our end users to loosen semantics of extensions over time than it is to tighten them. Jeff
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On Tue, 19 Mar 2019, Jeff Law wrote: > I'll note that our documentation clearly states that attributes can be > applied to functions, variables, labels, enums, statements and types. A key thing here is that they can be applied to fields - that is, they can be properties of a FIELD_DECL. Referring to a field either requires an expression referring to that field, or some other custom syntax that uses both a type name and a field name (like in __builtin_offsetof). -- Joseph S. Myers jos...@codesourcery.com
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
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
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On 2/22/19 9:42 AM, Marek Polacek wrote: > On Thu, Feb 21, 2019 at 03:39:21PM -0700, Martin Sebor wrote: >> On 2/21/19 1:27 PM, Jeff Law wrote: >>> On 2/21/19 1:12 PM, Martin Sebor wrote: On 2/21/19 12:08 PM, Jeff Law wrote: > On 2/18/19 7:53 PM, Martin Sebor wrote: >> Please let me know what it will take to get the fix for these two >> issues approved. I've answered the questions so I don't know what >> else I'm expected to do here. >> >> https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html > I think there is still a fundamental disagreement about whether or not > this should be handling expressions. Without an agreement on that it's > hard to see how this could go forward. I think it's wrong to hold up a fix for an ICE because you don't like the current design. The built-in successfully handles common expressions (see c-c++-common/builtin-has-attribute-5.c). It just fails for some of the less common ones. Not fixing those will only penalize users who run into the ICE, and cast a bad light on the quality of the release. Any design questions should be settled separately of these ICEs (should have been when the feature was being reviewed). That said, I have explained the rationale for the current design. Neither you nor Jakub has explained what you find wrong with it or why either of your alternatives is preferable. You said it should be an error to apply the built-in to expressions (why?). Jakub thought there perhaps should be two built-ins, one for expressions and another for decls. His rationale? The current design is not good. Clearly, the two of you don't agree on what you'd like to see; the only thing you agree on is that you don't like what's there now. What do you expect me to do with that, now at the end of stage 4? >>> Fix the ice in another way. Handling expressions here seems >>> fundamentally wrong. ISTM that making this query on an expression >>> should result in an immediate error. >> >> Sorry but "it seems wrong" is not a compelling argument. You need >> to explain what you think is wrong with it. > > "Attributes apply to decls and types, not expressions" seems like an > argument strong enough. (There are also special attributes for enums > and ';' and labels.) So I think Martin's argument is that for an expression we can look at the type of the expression and see if the attribute is on that type. That leads to some inconsistencies (that Jakub has pointed out) where an unfolded expression could produce a different result than it's folded result if the result was a constant. AFAICT the goal (outside handling just types) here is to be able to ask things like is a particular field within a structure packed, aliasing properties of objects, etc. My next thought was to use typeof to extract the type of the expression and pass that through (independent of Marek suggesting the same). Martin points out in a subsequent email that won't work because p->a in the supplied example returns an int rather than a packed int. One could argue that's the wrong return value for typeof, but I don't think it's that clear cut. In this specific instance I'd claim it's wrong, but I'd hazard a guess we don't have clear semantics in this space. I'll note that our documentation clearly states that attributes can be applied to functions, variables, labels, enums, statements and types. No provision AFAICT is made for expressions. Users have never had the ability or assumption that they can ask for attributes on an expression. Martin's patch essentially allows for asking indirectly -- but I suspect it's going to be inconsistent in more ways than Jakub pointed out as the type system in gimple is "loose" in certain ways and the result could well change as we go through the optimizer pipeline (ie, this is bigger than just constant folding causing inconsistencies). Given that we're talking about a new feature with no existing end user expectations and the fact that attributes are documented as applying only to things which are decls or types, I'm more inclined to stay conservative at this point and reject expressions as syntax errors. We can revisit in gcc-10 and see if there's a reasonable way to tighten semantics around typeof and/or loosen the requirement that we can only ask about attributes of DECL/TYPE nodes and define good semantics around that if we try. I'm also willing to revisit if other reviewers chime in with other opinions -- this kind of stuff isn't my strongest area. Jeff
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
I already explained the rationale behind the decision to have the built-in accept expressions: to be able to easily query for type attributes in expressions such as: typedef __attribute__ ((may_alias)) int* BadInt; void f (BadInt *p) { _Static_assert (__builtin_has_attribute (*p, may_alias)); } or struct S { int a[1] __attribute__ ((packed)); }; void f (struct S *p) { _Static_assert (__builtin_has_attribute (p->a, packed)); } or even _Static_assert (__builtin_has_attribute (p->a[0], packed)); If the built-in rejects expressions, I don't see how one would query for these properties. So how about __builtin_has_attribute (typeof (p->a), packed)? typeof (p->a) is int, not 'packed int', so the built-in returns false in this case, while true in the expression I wrote. This applies to any attribute that's attached to a member as opposed to its type. I know of no way to refer to a struct member in C without using some non-trivial expression (in C++, S::a could be used instead). Trying to query/set attributes on things like "1 + 1" just doesn't make any sense. So why don't we handle TYPE_P/DECL_P and give an error for the rest? Mainly because of the above. But also because there is no harm in accepting arbitrary expressions and querying their type, no problem that I'm aware of that would justify giving an error. This is analogous to __alignof__. If __alignof__ (1 + 1) makes enough sense to accept then I see no reason why __builtin_has_attribute (1 + 1, aligned) should not be. Martin PS __alignof__ is documented to determine the alignment requirement of a function, object, or a type, but it accepts expressions as well. For lvalues, like p->a above, __alignof__ is further documented to determine the alignment of the lvalue's type, but that's not what it actually does, at least not in the sense of __alignof__ (__typeof__ (p->a)), for over- or underaligned struct members declared either with attribute aligned or packed. What it does is return the alignment of the member subobject, which is, I'm sure, what most users expect.
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On Thu, Feb 21, 2019 at 03:39:21PM -0700, Martin Sebor wrote: > On 2/21/19 1:27 PM, Jeff Law wrote: > > On 2/21/19 1:12 PM, Martin Sebor wrote: > > > On 2/21/19 12:08 PM, Jeff Law wrote: > > > > On 2/18/19 7:53 PM, Martin Sebor wrote: > > > > > Please let me know what it will take to get the fix for these two > > > > > issues approved. I've answered the questions so I don't know what > > > > > else I'm expected to do here. > > > > > > > > > > https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html > > > > I think there is still a fundamental disagreement about whether or not > > > > this should be handling expressions. Without an agreement on that it's > > > > hard to see how this could go forward. > > > > > > I think it's wrong to hold up a fix for an ICE because you don't > > > like the current design. The built-in successfully handles common > > > expressions (see c-c++-common/builtin-has-attribute-5.c). It just > > > fails for some of the less common ones. Not fixing those will only > > > penalize users who run into the ICE, and cast a bad light on > > > the quality of the release. Any design questions should be > > > settled separately of these ICEs (should have been when > > > the feature was being reviewed). > > > > > > That said, I have explained the rationale for the current design. > > > Neither you nor Jakub has explained what you find wrong with it or > > > why either of your alternatives is preferable. You said it should > > > be an error to apply the built-in to expressions (why?). Jakub > > > thought there perhaps should be two built-ins, one for expressions > > > and another for decls. His rationale? The current design is not > > > good. Clearly, the two of you don't agree on what you'd like to > > > see; the only thing you agree on is that you don't like what's > > > there now. What do you expect me to do with that, now at the end > > > of stage 4? > > Fix the ice in another way. Handling expressions here seems > > fundamentally wrong. ISTM that making this query on an expression > > should result in an immediate error. > > Sorry but "it seems wrong" is not a compelling argument. You need > to explain what you think is wrong with it. "Attributes apply to decls and types, not expressions" seems like an argument strong enough. (There are also special attributes for enums and ';' and labels.) > The built-in is modeled on the sizeof, alignof, and typeof operators. > The manual documents like this: > > bool __builtin_has_attribute (type-or-expression, attribute) > > ...The type-or-expression argument is subject to the same > restrictions as the argument to typeof (see Typeof). > > It was reviewed and approved with no objections to the API. So it > seems that it's up to you to provide a convincing argument that this > was the wrong choice. What serious problems do you think it might > cause that justify rejecting expressions, and how will users overcome > this limitation? > > I already explained the rationale behind the decision to have > the built-in accept expressions: to be able to easily query for > type attributes in expressions such as: > > typedef __attribute__ ((may_alias)) int* BadInt; > > void f (BadInt *p) > { > _Static_assert (__builtin_has_attribute (*p, may_alias)); > } > > or > > struct S { > int a[1] __attribute__ ((packed)); > }; > > void f (struct S *p) > { > _Static_assert (__builtin_has_attribute (p->a, packed)); > } > > or even > > _Static_assert (__builtin_has_attribute (p->a[0], packed)); > > If the built-in rejects expressions, I don't see how one would > query for these properties. So how about __builtin_has_attribute (typeof (p->a), packed)? Trying to query/set attributes on things like "1 + 1" just doesn't make any sense. So why don't we handle TYPE_P/DECL_P and give an error for the rest? Marek
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On 2/21/19 3:42 PM, Jeff Law wrote: On 2/21/19 3:39 PM, Martin Sebor wrote: On 2/21/19 1:27 PM, Jeff Law wrote: On 2/21/19 1:12 PM, Martin Sebor wrote: On 2/21/19 12:08 PM, Jeff Law wrote: On 2/18/19 7:53 PM, Martin Sebor wrote: Please let me know what it will take to get the fix for these two issues approved. I've answered the questions so I don't know what else I'm expected to do here. https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html I think there is still a fundamental disagreement about whether or not this should be handling expressions. Without an agreement on that it's hard to see how this could go forward. I think it's wrong to hold up a fix for an ICE because you don't like the current design. The built-in successfully handles common expressions (see c-c++-common/builtin-has-attribute-5.c). It just fails for some of the less common ones. Not fixing those will only penalize users who run into the ICE, and cast a bad light on the quality of the release. Any design questions should be settled separately of these ICEs (should have been when the feature was being reviewed). That said, I have explained the rationale for the current design. Neither you nor Jakub has explained what you find wrong with it or why either of your alternatives is preferable. You said it should be an error to apply the built-in to expressions (why?). Jakub thought there perhaps should be two built-ins, one for expressions and another for decls. His rationale? The current design is not good. Clearly, the two of you don't agree on what you'd like to see; the only thing you agree on is that you don't like what's there now. What do you expect me to do with that, now at the end of stage 4? Fix the ice in another way. Handling expressions here seems fundamentally wrong. ISTM that making this query on an expression should result in an immediate error. Sorry but "it seems wrong" is not a compelling argument. You need to explain what you think is wrong with it. Attributes on expressions seems fundamentally broken. Jakub has raised this issue as well. If you want things to move forward you need to address this fundamental issue. I need to understand what this fundamental problem is. The built-in is modeled on the sizeof, alignof, and typeof operators. The manual documents like this: Understood, but that makes it rather different from most (all?) other attributes. Attributes apply to decls and types, not expressions. bool __builtin_has_attribute (type-or-expression, attribute) ...The type-or-expression argument is subject to the same restrictions as the argument to typeof (see Typeof). It was reviewed and approved with no objections to the API. ANd I botched that. Mistakes happen. It was also reviewed by Joseph and Jason. Can you show how one would query for attribute packed in the example I gave if the built-in were to reject expressions: struct S { int a[1] __attribute__ ((packed)); }; void f (struct S *p) { _Static_assert (__builtin_has_attribute (p->a, packed)); } In any event, if there is a problem I will certainly fix it. But I need to know what the problem is first. Martin
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On 2/21/19 3:39 PM, Martin Sebor wrote: > On 2/21/19 1:27 PM, Jeff Law wrote: >> On 2/21/19 1:12 PM, Martin Sebor wrote: >>> On 2/21/19 12:08 PM, Jeff Law wrote: On 2/18/19 7:53 PM, Martin Sebor wrote: > Please let me know what it will take to get the fix for these two > issues approved. I've answered the questions so I don't know what > else I'm expected to do here. > > https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html I think there is still a fundamental disagreement about whether or not this should be handling expressions. Without an agreement on that it's hard to see how this could go forward. >>> >>> I think it's wrong to hold up a fix for an ICE because you don't >>> like the current design. The built-in successfully handles common >>> expressions (see c-c++-common/builtin-has-attribute-5.c). It just >>> fails for some of the less common ones. Not fixing those will only >>> penalize users who run into the ICE, and cast a bad light on >>> the quality of the release. Any design questions should be >>> settled separately of these ICEs (should have been when >>> the feature was being reviewed). >>> >>> That said, I have explained the rationale for the current design. >>> Neither you nor Jakub has explained what you find wrong with it or >>> why either of your alternatives is preferable. You said it should >>> be an error to apply the built-in to expressions (why?). Jakub >>> thought there perhaps should be two built-ins, one for expressions >>> and another for decls. His rationale? The current design is not >>> good. Clearly, the two of you don't agree on what you'd like to >>> see; the only thing you agree on is that you don't like what's >>> there now. What do you expect me to do with that, now at the end >>> of stage 4? >> Fix the ice in another way. Handling expressions here seems >> fundamentally wrong. ISTM that making this query on an expression >> should result in an immediate error. > > Sorry but "it seems wrong" is not a compelling argument. You need > to explain what you think is wrong with it. Attributes on expressions seems fundamentally broken. Jakub has raised this issue as well. If you want things to move forward you need to address this fundamental issue. > > The built-in is modeled on the sizeof, alignof, and typeof operators. > The manual documents like this: Understood, but that makes it rather different from most (all?) other attributes. Attributes apply to decls and types, not expressions. > > bool __builtin_has_attribute (type-or-expression, attribute) > > ...The type-or-expression argument is subject to the same > restrictions as the argument to typeof (see Typeof). > > It was reviewed and approved with no objections to the API. ANd I botched that. Mistakes happen. Jeff
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On 2/21/19 1:27 PM, Jeff Law wrote: On 2/21/19 1:12 PM, Martin Sebor wrote: On 2/21/19 12:08 PM, Jeff Law wrote: On 2/18/19 7:53 PM, Martin Sebor wrote: Please let me know what it will take to get the fix for these two issues approved. I've answered the questions so I don't know what else I'm expected to do here. https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html I think there is still a fundamental disagreement about whether or not this should be handling expressions. Without an agreement on that it's hard to see how this could go forward. I think it's wrong to hold up a fix for an ICE because you don't like the current design. The built-in successfully handles common expressions (see c-c++-common/builtin-has-attribute-5.c). It just fails for some of the less common ones. Not fixing those will only penalize users who run into the ICE, and cast a bad light on the quality of the release. Any design questions should be settled separately of these ICEs (should have been when the feature was being reviewed). That said, I have explained the rationale for the current design. Neither you nor Jakub has explained what you find wrong with it or why either of your alternatives is preferable. You said it should be an error to apply the built-in to expressions (why?). Jakub thought there perhaps should be two built-ins, one for expressions and another for decls. His rationale? The current design is not good. Clearly, the two of you don't agree on what you'd like to see; the only thing you agree on is that you don't like what's there now. What do you expect me to do with that, now at the end of stage 4? Fix the ice in another way. Handling expressions here seems fundamentally wrong. ISTM that making this query on an expression should result in an immediate error. Sorry but "it seems wrong" is not a compelling argument. You need to explain what you think is wrong with it. The built-in is modeled on the sizeof, alignof, and typeof operators. The manual documents like this: bool __builtin_has_attribute (type-or-expression, attribute) ...The type-or-expression argument is subject to the same restrictions as the argument to typeof (see Typeof). It was reviewed and approved with no objections to the API. So it seems that it's up to you to provide a convincing argument that this was the wrong choice. What serious problems do you think it might cause that justify rejecting expressions, and how will users overcome this limitation? I already explained the rationale behind the decision to have the built-in accept expressions: to be able to easily query for type attributes in expressions such as: typedef __attribute__ ((may_alias)) int* BadInt; void f (BadInt *p) { _Static_assert (__builtin_has_attribute (*p, may_alias)); } or struct S { int a[1] __attribute__ ((packed)); }; void f (struct S *p) { _Static_assert (__builtin_has_attribute (p->a, packed)); } or even _Static_assert (__builtin_has_attribute (p->a[0], packed)); If the built-in rejects expressions, I don't see how one would query for these properties. Martin
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On 2/21/19 1:12 PM, Martin Sebor wrote: > On 2/21/19 12:08 PM, Jeff Law wrote: >> On 2/18/19 7:53 PM, Martin Sebor wrote: >>> Please let me know what it will take to get the fix for these two >>> issues approved. I've answered the questions so I don't know what >>> else I'm expected to do here. >>> >>> https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html >> I think there is still a fundamental disagreement about whether or not >> this should be handling expressions. Without an agreement on that it's >> hard to see how this could go forward. > > I think it's wrong to hold up a fix for an ICE because you don't > like the current design. The built-in successfully handles common > expressions (see c-c++-common/builtin-has-attribute-5.c). It just > fails for some of the less common ones. Not fixing those will only > penalize users who run into the ICE, and cast a bad light on > the quality of the release. Any design questions should be > settled separately of these ICEs (should have been when > the feature was being reviewed). > > That said, I have explained the rationale for the current design. > Neither you nor Jakub has explained what you find wrong with it or > why either of your alternatives is preferable. You said it should > be an error to apply the built-in to expressions (why?). Jakub > thought there perhaps should be two built-ins, one for expressions > and another for decls. His rationale? The current design is not > good. Clearly, the two of you don't agree on what you'd like to > see; the only thing you agree on is that you don't like what's > there now. What do you expect me to do with that, now at the end > of stage 4? Fix the ice in another way. Handling expressions here seems fundamentally wrong. ISTM that making this query on an expression should result in an immediate error. jeff
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
On 2/21/19 12:08 PM, Jeff Law wrote: On 2/18/19 7:53 PM, Martin Sebor wrote: Please let me know what it will take to get the fix for these two issues approved. I've answered the questions so I don't know what else I'm expected to do here. https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00793.html I think there is still a fundamental disagreement about whether or not this should be handling expressions. Without an agreement on that it's hard to see how this could go forward. I think it's wrong to hold up a fix for an ICE because you don't like the current design. The built-in successfully handles common expressions (see c-c++-common/builtin-has-attribute-5.c). It just fails for some of the less common ones. Not fixing those will only penalize users who run into the ICE, and cast a bad light on the quality of the release. Any design questions should be settled separately of these ICEs (should have been when the feature was being reviewed). That said, I have explained the rationale for the current design. Neither you nor Jakub has explained what you find wrong with it or why either of your alternatives is preferable. You said it should be an error to apply the built-in to expressions (why?). Jakub thought there perhaps should be two built-ins, one for expressions and another for decls. His rationale? The current design is not good. Clearly, the two of you don't agree on what you'd like to see; the only thing you agree on is that you don't like what's there now. What do you expect me to do with that, now at the end of stage 4? Martin
Re: PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
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
PING [PATCH] fix ICE in __builtin_has_attribute (PR 88383 and 89288)
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