Re: PING #2 [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
On 1/15/19 9:31 AM, Martin Sebor wrote: > Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html > > Jeff, do you have any more questions/concerns or is this patch > good to commit? I think both Jakub and I were concerned about handling expressions in this code. I don't recall a resolution on that fundamental question. jeff
Re: PING #2 [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
Jeff, do you have any more questions/concerns or is this patch
good to commit?
Martin
On 1/7/19 5:32 PM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
On 1/3/19 3:22 PM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
On 12/20/18 7:51 PM, Martin Sebor wrote:
Jeff, did this and the rest of the discussion answer your question
and if so, is the patch okay to commit?
https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
Martin
On 12/13/18 12:48 PM, Martin Sebor wrote:
On 12/13/18 12:20 PM, Martin Sebor wrote:
On 12/13/18 11:59 AM, Jeff Law wrote:
On 12/5/18 8:55 PM, Martin Sebor wrote:
The __builtin_has_attribute function fails with an ICE when
its first argument is an expression such as INDIRECT_REF, or
many others. The code simply assumes it's either a type or
a decl. The attached patch corrects this oversight.
While testing the fix I also noticed the C++ front end expects
the first operand to be a unary expression, which causes most
other kinds of expressions to be rejected. The patch fixes
that as well.
Finally, while testing the fix even more I realized that
the built-in considers the underlying array itself in ARRAY_REF
expressions rather than its type, which leads to inconsistent
results for overaligned arrays (it's the array itself that's
overaligned, not its elements). So I fixed that too and
adjusted the one test designed to verify this.
Tested on x86_64-linux.
Martin
gcc-88383.diff
PR c/88383 - ICE calling __builtin_has_attribute on a reference
gcc/c-family/ChangeLog:
PR c/88383
* c-attribs.c (validate_attribute): Handle expressions.
(has_attribute): Handle types referenced by expressions.
Avoid considering array attributes in ARRAY_REF expressions .
gcc/cp/ChangeLog:
PR c/88383
* parser.c (cp_parser_has_attribute_expression): Handle
assignment
expressions.
gcc/testsuite/ChangeLog:
PR c/88383
* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
* c-c++-common/builtin-has-attribute-6.c: New test.
Well, the high level question here is do we want to support this
builtin
on expressions at all. Generally attributes apply to types and
decls,
not expressions.
Clearly we shouldn't fault, but my first inclination is that the
attribute applies to types or decls, not expressions. In that
case we
should just be issuing an error.
I could be convinced otherwise, so if you think we should support
passing expressions to this builtin, make your case.
The support is necessary in order to determine the attributes
in expressions such as:
struct S { __attribute__ ((packed)) int a[32]; };
extern struct S s;
_Static_assert (__builtin_has_attribute (s.a, packed));
An example involving types might be a better one:
typedef __attribute__ ((may_alias)) int* BadInt;
void f (BadInt *p)
{
_Static_assert (__builtin_has_attribute (*p, may_alias));
}
Martin
PING #2 [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
On 1/3/19 3:22 PM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
On 12/20/18 7:51 PM, Martin Sebor wrote:
Jeff, did this and the rest of the discussion answer your question
and if so, is the patch okay to commit?
https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
Martin
On 12/13/18 12:48 PM, Martin Sebor wrote:
On 12/13/18 12:20 PM, Martin Sebor wrote:
On 12/13/18 11:59 AM, Jeff Law wrote:
On 12/5/18 8:55 PM, Martin Sebor wrote:
The __builtin_has_attribute function fails with an ICE when
its first argument is an expression such as INDIRECT_REF, or
many others. The code simply assumes it's either a type or
a decl. The attached patch corrects this oversight.
While testing the fix I also noticed the C++ front end expects
the first operand to be a unary expression, which causes most
other kinds of expressions to be rejected. The patch fixes
that as well.
Finally, while testing the fix even more I realized that
the built-in considers the underlying array itself in ARRAY_REF
expressions rather than its type, which leads to inconsistent
results for overaligned arrays (it's the array itself that's
overaligned, not its elements). So I fixed that too and
adjusted the one test designed to verify this.
Tested on x86_64-linux.
Martin
gcc-88383.diff
PR c/88383 - ICE calling __builtin_has_attribute on a reference
gcc/c-family/ChangeLog:
PR c/88383
* c-attribs.c (validate_attribute): Handle expressions.
(has_attribute): Handle types referenced by expressions.
Avoid considering array attributes in ARRAY_REF expressions .
gcc/cp/ChangeLog:
PR c/88383
* parser.c (cp_parser_has_attribute_expression): Handle
assignment
expressions.
gcc/testsuite/ChangeLog:
PR c/88383
* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
* c-c++-common/builtin-has-attribute-6.c: New test.
Well, the high level question here is do we want to support this
builtin
on expressions at all. Generally attributes apply to types and decls,
not expressions.
Clearly we shouldn't fault, but my first inclination is that the
attribute applies to types or decls, not expressions. In that case we
should just be issuing an error.
I could be convinced otherwise, so if you think we should support
passing expressions to this builtin, make your case.
The support is necessary in order to determine the attributes
in expressions such as:
struct S { __attribute__ ((packed)) int a[32]; };
extern struct S s;
_Static_assert (__builtin_has_attribute (s.a, packed));
An example involving types might be a better one:
typedef __attribute__ ((may_alias)) int* BadInt;
void f (BadInt *p)
{
_Static_assert (__builtin_has_attribute (*p, may_alias));
}
Martin
PING [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
On 12/20/18 7:51 PM, Martin Sebor wrote:
Jeff, did this and the rest of the discussion answer your question
and if so, is the patch okay to commit?
https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
Martin
On 12/13/18 12:48 PM, Martin Sebor wrote:
On 12/13/18 12:20 PM, Martin Sebor wrote:
On 12/13/18 11:59 AM, Jeff Law wrote:
On 12/5/18 8:55 PM, Martin Sebor wrote:
The __builtin_has_attribute function fails with an ICE when
its first argument is an expression such as INDIRECT_REF, or
many others. The code simply assumes it's either a type or
a decl. The attached patch corrects this oversight.
While testing the fix I also noticed the C++ front end expects
the first operand to be a unary expression, which causes most
other kinds of expressions to be rejected. The patch fixes
that as well.
Finally, while testing the fix even more I realized that
the built-in considers the underlying array itself in ARRAY_REF
expressions rather than its type, which leads to inconsistent
results for overaligned arrays (it's the array itself that's
overaligned, not its elements). So I fixed that too and
adjusted the one test designed to verify this.
Tested on x86_64-linux.
Martin
gcc-88383.diff
PR c/88383 - ICE calling __builtin_has_attribute on a reference
gcc/c-family/ChangeLog:
PR c/88383
* c-attribs.c (validate_attribute): Handle expressions.
(has_attribute): Handle types referenced by expressions.
Avoid considering array attributes in ARRAY_REF expressions .
gcc/cp/ChangeLog:
PR c/88383
* parser.c (cp_parser_has_attribute_expression): Handle assignment
expressions.
gcc/testsuite/ChangeLog:
PR c/88383
* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
* c-c++-common/builtin-has-attribute-6.c: New test.
Well, the high level question here is do we want to support this
builtin
on expressions at all. Generally attributes apply to types and decls,
not expressions.
Clearly we shouldn't fault, but my first inclination is that the
attribute applies to types or decls, not expressions. In that case we
should just be issuing an error.
I could be convinced otherwise, so if you think we should support
passing expressions to this builtin, make your case.
The support is necessary in order to determine the attributes
in expressions such as:
struct S { __attribute__ ((packed)) int a[32]; };
extern struct S s;
_Static_assert (__builtin_has_attribute (s.a, packed));
An example involving types might be a better one:
typedef __attribute__ ((may_alias)) int* BadInt;
void f (BadInt *p)
{
_Static_assert (__builtin_has_attribute (*p, may_alias));
}
Martin
Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
Jeff, did this and the rest of the discussion answer your question
and if so, is the patch okay to commit?
https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00337.html
Martin
On 12/13/18 12:48 PM, Martin Sebor wrote:
On 12/13/18 12:20 PM, Martin Sebor wrote:
On 12/13/18 11:59 AM, Jeff Law wrote:
On 12/5/18 8:55 PM, Martin Sebor wrote:
The __builtin_has_attribute function fails with an ICE when
its first argument is an expression such as INDIRECT_REF, or
many others. The code simply assumes it's either a type or
a decl. The attached patch corrects this oversight.
While testing the fix I also noticed the C++ front end expects
the first operand to be a unary expression, which causes most
other kinds of expressions to be rejected. The patch fixes
that as well.
Finally, while testing the fix even more I realized that
the built-in considers the underlying array itself in ARRAY_REF
expressions rather than its type, which leads to inconsistent
results for overaligned arrays (it's the array itself that's
overaligned, not its elements). So I fixed that too and
adjusted the one test designed to verify this.
Tested on x86_64-linux.
Martin
gcc-88383.diff
PR c/88383 - ICE calling __builtin_has_attribute on a reference
gcc/c-family/ChangeLog:
PR c/88383
* c-attribs.c (validate_attribute): Handle expressions.
(has_attribute): Handle types referenced by expressions.
Avoid considering array attributes in ARRAY_REF expressions .
gcc/cp/ChangeLog:
PR c/88383
* parser.c (cp_parser_has_attribute_expression): Handle assignment
expressions.
gcc/testsuite/ChangeLog:
PR c/88383
* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
* c-c++-common/builtin-has-attribute-6.c: New test.
Well, the high level question here is do we want to support this builtin
on expressions at all. Generally attributes apply to types and decls,
not expressions.
Clearly we shouldn't fault, but my first inclination is that the
attribute applies to types or decls, not expressions. In that case we
should just be issuing an error.
I could be convinced otherwise, so if you think we should support
passing expressions to this builtin, make your case.
The support is necessary in order to determine the attributes
in expressions such as:
struct S { __attribute__ ((packed)) int a[32]; };
extern struct S s;
_Static_assert (__builtin_has_attribute (s.a, packed));
An example involving types might be a better one:
typedef __attribute__ ((may_alias)) int* BadInt;
void f (BadInt *p)
{
_Static_assert (__builtin_has_attribute (*p, may_alias));
}
Martin
Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
the manual a function declaration like __attribute__ ((alloc_size (1), malloc)) void* allocate (unsigned); should have those two attributes applied to it. Yet, alloc_size is actually applied to its type (but not to its decl) while malloc to the function's decl but not its type (bug 88397). I'm pretty sure most users still expect the following to pass: _Static_assert (__builtin_has_attribute (allocate, alloc_size)); _Static_assert (__builtin_has_attribute (allocate, malloc)); Users shouldn't expect this. If anything, we should document what attributes are type attributes and which attributes are declaration attributes. I designed the built-in based on what I expect. The programs that care about whether a function is declared, say with attribute alloc_align or malloc, do not and should not have to worry about whether the attribute is on the decl or on its type -- in the expected use cases it makes no difference. Those that might care whether an attribute is on the type can easily check the type: __builtin_has_attribute (__typeof__ (allocate), alloc_size) (I would expect GCC to apply an attribute either to a decl or to a type but not to both.) I do agree that whether an attribute applies to a function or its type should be reviewed and where it makes sense documented. More than that, some attributes that currently only apply to function decls should be changed to apply to (function) types instead so that calls via pointers to such functions can get the same benefits as calls to the functions themselves. Malloc is an example (bug 88397). With the way you're proposing, users could check the type attributes simply through __typeof/decltype etc., but couldn't test solely the declaration attributes. I'm not proposing anything -- what I described is the design. I don't know of a use case for testing the decl alone for attributes. In all those I can think of, what matters is the union of attributes between the decl and its type. But I'm not opposed to enhancing the function if an important use case does turn up that's not supported. For what you are asking for this should already do it so I don't see a need to change anything: #define decl_has_attribute(d, ...) \ (__builtin_has_attribute (d, __VA_ARGS__) \ && !__builtin_has_attribute (__typeof__ (d), __VA_ARGS__)) Martin
Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
On Thu, Dec 13, 2018 at 09:03:57PM -0700, Martin Sebor wrote: > It's as good as the design of GCC attributes allows. Based on No. > the manual a function declaration like > > __attribute__ ((alloc_size (1), malloc)) > void* allocate (unsigned); > > should have those two attributes applied to it. Yet, alloc_size > is actually applied to its type (but not to its decl) while malloc > to the function's decl but not its type (bug 88397). > > I'm pretty sure most users still expect the following to pass: > > _Static_assert (__builtin_has_attribute (allocate, alloc_size)); > _Static_assert (__builtin_has_attribute (allocate, malloc)); Users shouldn't expect this. If anything, we should document what attributes are type attributes and which attributes are declaration attributes. With the way you're proposing, users could check the type attributes simply through __typeof/decltype etc., but couldn't test solely the declaration attributes. Jakub
Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
On 12/13/18 4:39 PM, Jakub Jelinek wrote: On Thu, Dec 13, 2018 at 02:05:29PM -0700, Martin Sebor wrote: So how is the builtin defined then? Is the argument always an expression and you only return whether its type has the attribute, or do something different if the expression is of certain kind? For a DECL the built-in looks at both the DECL_ATTRIBUTES and the DECL's type's TYPE_ATTRIBUTES. For expressions, it just looks at TYPE_ATTRIBUTES of the expression's type. I'm not sure that this is necessarily the cleanest design but it's meant to follow how attributes are applied to DECLs. Sometimes they get applied to the DECL itself and other times to its type. This has been causing confusion to both users and GCC devs such as myself so at some point I'd like to make this clearer somehow. Not sure exactly how yet. But some users of the attribute look for the attribute on TYPE_ATTRIBUTES, other on DECL_ATTRIBUTES, and the attributes have bools which say to what they apply. So, I think the API need to be very clear whether the attribute is on a decl or on a type. If it is similar to say sizeof/__alignof__ etc., where it accepts either a type (for which one can use __typeof/decltype etc.), in which case it would check the type attributes, or an expression and in that case require it to be a decl and use decl attributes, one API is fine, but supporting arbitrary expressions and sometimes looking at type, other times at decl or both is not a good design. It's as good as the design of GCC attributes allows. Based on the manual a function declaration like __attribute__ ((alloc_size (1), malloc)) void* allocate (unsigned); should have those two attributes applied to it. Yet, alloc_size is actually applied to its type (but not to its decl) while malloc to the function's decl but not its type (bug 88397). I'm pretty sure most users still expect the following to pass: _Static_assert (__builtin_has_attribute (allocate, alloc_size)); _Static_assert (__builtin_has_attribute (allocate, malloc)); It wouldn't if the built-in differentiated between decls and types. Even if/when we make these function attributes (and others like it) apply to types as I think we should so it's possible to apply them to function pointers as users expect, there still will be others that will apply only to the function decls. The distinction between when GCC chooses to apply an attribute to a function decl vs to its type is not intuitive and cannot very well be, and neither would be an API that queried just a decl attribute or just a type attribute. The most common use case is to query whether a function has been declared with an attribute -- it doesn't matter whether it's on its unique type or on its decl. (The others might perhaps be useful too, but a lot less often.) Martin
Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
On Thu, Dec 13, 2018 at 02:05:29PM -0700, Martin Sebor wrote: > > So how is the builtin defined then? Is the argument always an expression > > and you only return whether its type has the attribute, or do something > > different if the expression is of certain kind? > > For a DECL the built-in looks at both the DECL_ATTRIBUTES and > the DECL's type's TYPE_ATTRIBUTES. For expressions, it just > looks at TYPE_ATTRIBUTES of the expression's type. I'm not > sure that this is necessarily the cleanest design but it's > meant to follow how attributes are applied to DECLs. Sometimes > they get applied to the DECL itself and other times to its type. > This has been causing confusion to both users and GCC devs such > as myself so at some point I'd like to make this clearer somehow. > Not sure exactly how yet. But some users of the attribute look for the attribute on TYPE_ATTRIBUTES, other on DECL_ATTRIBUTES, and the attributes have bools which say to what they apply. So, I think the API need to be very clear whether the attribute is on a decl or on a type. If it is similar to say sizeof/__alignof__ etc., where it accepts either a type (for which one can use __typeof/decltype etc.), in which case it would check the type attributes, or an expression and in that case require it to be a decl and use decl attributes, one API is fine, but supporting arbitrary expressions and sometimes looking at type, other times at decl or both is not a good design. Jakub
Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
On 12/13/18 12:56 PM, Jakub Jelinek wrote:
On Thu, Dec 13, 2018 at 12:48:18PM -0700, Martin Sebor wrote:
The support is necessary in order to determine the attributes
in expressions such as:
struct S { __attribute__ ((packed)) int a[32]; };
extern struct S s;
_Static_assert (__builtin_has_attribute (s.a, packed));
An example involving types might be a better one:
typedef __attribute__ ((may_alias)) int* BadInt;
void f (BadInt *p)
{
_Static_assert (__builtin_has_attribute (*p, may_alias));
}
So how is the builtin defined then? Is the argument always an expression
and you only return whether its type has the attribute, or do something
different if the expression is of certain kind?
For a DECL the built-in looks at both the DECL_ATTRIBUTES and
the DECL's type's TYPE_ATTRIBUTES. For expressions, it just
looks at TYPE_ATTRIBUTES of the expression's type. I'm not
sure that this is necessarily the cleanest design but it's
meant to follow how attributes are applied to DECLs. Sometimes
they get applied to the DECL itself and other times to its type.
This has been causing confusion to both users and GCC devs such
as myself so at some point I'd like to make this clearer somehow.
Not sure exactly how yet.
Perhaps it would be better have two builtins, one would always take
type from the expression, the other would only accept decls (or perhaps
some syntax for the FIELD_DECLs).
I'm not sure I see a need for two built-ins here.
There is another way to handle the may_alias example: by using
typeof first and then the built-in on the result:
_Static_assert (__builtin_has_attribute (__typeof__ (*p), may_alias));
so it seems that it would be possible to do what Jeff suggests
and only accept DECLs and TYPEs. But I'm also not sure I see
an advantage of restricting the built-in like that.
FWIW, I had in mind two uses when I introduced the built-in:
1) a conditional attribute copy (where the condition would be
whether or not the source DECL or TYPE has some attribute), and
2) detecting that attributes have been successfully applied as
expected in tests. I haven't implemented (1) yet but I'd like
to at some point (if only to avoid hardcoding into GCC the set
of attributes that are excluded from copying). I have added
a handful of tests that make use of the built-in. So if changes
should be made to the built-in, I will want to make sure they
don't make these use cases difficult.
Martin
Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
On Thu, Dec 13, 2018 at 12:48:18PM -0700, Martin Sebor wrote:
> > The support is necessary in order to determine the attributes
> > in expressions such as:
> >
> > struct S { __attribute__ ((packed)) int a[32]; };
> >
> > extern struct S s;
> >
> > _Static_assert (__builtin_has_attribute (s.a, packed));
>
> An example involving types might be a better one:
>
> typedef __attribute__ ((may_alias)) int* BadInt;
>
> void f (BadInt *p)
> {
> _Static_assert (__builtin_has_attribute (*p, may_alias));
> }
So how is the builtin defined then? Is the argument always an expression
and you only return whether its type has the attribute, or do something
different if the expression is of certain kind?
Perhaps it would be better have two builtins, one would always take
type from the expression, the other would only accept decls (or perhaps
some syntax for the FIELD_DECLs).
Jakub
Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
On 12/13/18 12:20 PM, Martin Sebor wrote:
On 12/13/18 11:59 AM, Jeff Law wrote:
On 12/5/18 8:55 PM, Martin Sebor wrote:
The __builtin_has_attribute function fails with an ICE when
its first argument is an expression such as INDIRECT_REF, or
many others. The code simply assumes it's either a type or
a decl. The attached patch corrects this oversight.
While testing the fix I also noticed the C++ front end expects
the first operand to be a unary expression, which causes most
other kinds of expressions to be rejected. The patch fixes
that as well.
Finally, while testing the fix even more I realized that
the built-in considers the underlying array itself in ARRAY_REF
expressions rather than its type, which leads to inconsistent
results for overaligned arrays (it's the array itself that's
overaligned, not its elements). So I fixed that too and
adjusted the one test designed to verify this.
Tested on x86_64-linux.
Martin
gcc-88383.diff
PR c/88383 - ICE calling __builtin_has_attribute on a reference
gcc/c-family/ChangeLog:
PR c/88383
* c-attribs.c (validate_attribute): Handle expressions.
(has_attribute): Handle types referenced by expressions.
Avoid considering array attributes in ARRAY_REF expressions .
gcc/cp/ChangeLog:
PR c/88383
* parser.c (cp_parser_has_attribute_expression): Handle assignment
expressions.
gcc/testsuite/ChangeLog:
PR c/88383
* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
* c-c++-common/builtin-has-attribute-6.c: New test.
Well, the high level question here is do we want to support this builtin
on expressions at all. Generally attributes apply to types and decls,
not expressions.
Clearly we shouldn't fault, but my first inclination is that the
attribute applies to types or decls, not expressions. In that case we
should just be issuing an error.
I could be convinced otherwise, so if you think we should support
passing expressions to this builtin, make your case.
The support is necessary in order to determine the attributes
in expressions such as:
struct S { __attribute__ ((packed)) int a[32]; };
extern struct S s;
_Static_assert (__builtin_has_attribute (s.a, packed));
An example involving types might be a better one:
typedef __attribute__ ((may_alias)) int* BadInt;
void f (BadInt *p)
{
_Static_assert (__builtin_has_attribute (*p, may_alias));
}
Martin
Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
On 12/13/18 11:59 AM, Jeff Law wrote:
On 12/5/18 8:55 PM, Martin Sebor wrote:
The __builtin_has_attribute function fails with an ICE when
its first argument is an expression such as INDIRECT_REF, or
many others. The code simply assumes it's either a type or
a decl. The attached patch corrects this oversight.
While testing the fix I also noticed the C++ front end expects
the first operand to be a unary expression, which causes most
other kinds of expressions to be rejected. The patch fixes
that as well.
Finally, while testing the fix even more I realized that
the built-in considers the underlying array itself in ARRAY_REF
expressions rather than its type, which leads to inconsistent
results for overaligned arrays (it's the array itself that's
overaligned, not its elements). So I fixed that too and
adjusted the one test designed to verify this.
Tested on x86_64-linux.
Martin
gcc-88383.diff
PR c/88383 - ICE calling __builtin_has_attribute on a reference
gcc/c-family/ChangeLog:
PR c/88383
* c-attribs.c (validate_attribute): Handle expressions.
(has_attribute): Handle types referenced by expressions.
Avoid considering array attributes in ARRAY_REF expressions .
gcc/cp/ChangeLog:
PR c/88383
* parser.c (cp_parser_has_attribute_expression): Handle assignment
expressions.
gcc/testsuite/ChangeLog:
PR c/88383
* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
* c-c++-common/builtin-has-attribute-6.c: New test.
Well, the high level question here is do we want to support this builtin
on expressions at all. Generally attributes apply to types and decls,
not expressions.
Clearly we shouldn't fault, but my first inclination is that the
attribute applies to types or decls, not expressions. In that case we
should just be issuing an error.
I could be convinced otherwise, so if you think we should support
passing expressions to this builtin, make your case.
The support is necessary in order to determine the attributes
in expressions such as:
struct S { __attribute__ ((packed)) int a[32]; };
extern struct S s;
_Static_assert (__builtin_has_attribute (s.a, packed));
Martin
Re: [PATCH] handle expressions in __builtin_has_attribute (PR 88383)
On 12/5/18 8:55 PM, Martin Sebor wrote: > The __builtin_has_attribute function fails with an ICE when > its first argument is an expression such as INDIRECT_REF, or > many others. The code simply assumes it's either a type or > a decl. The attached patch corrects this oversight. > > While testing the fix I also noticed the C++ front end expects > the first operand to be a unary expression, which causes most > other kinds of expressions to be rejected. The patch fixes > that as well. > > Finally, while testing the fix even more I realized that > the built-in considers the underlying array itself in ARRAY_REF > expressions rather than its type, which leads to inconsistent > results for overaligned arrays (it's the array itself that's > overaligned, not its elements). So I fixed that too and > adjusted the one test designed to verify this. > > Tested on x86_64-linux. > > Martin > > gcc-88383.diff > > PR c/88383 - ICE calling __builtin_has_attribute on a reference > > gcc/c-family/ChangeLog: > > PR c/88383 > * c-attribs.c (validate_attribute): Handle expressions. > (has_attribute): Handle types referenced by expressions. > Avoid considering array attributes in ARRAY_REF expressions . > > gcc/cp/ChangeLog: > > PR c/88383 > * parser.c (cp_parser_has_attribute_expression): Handle assignment > expressions. > > gcc/testsuite/ChangeLog: > > PR c/88383 > * c-c++-common/builtin-has-attribute-4.c: Adjust expectations. > * c-c++-common/builtin-has-attribute-6.c: New test. Well, the high level question here is do we want to support this builtin on expressions at all. Generally attributes apply to types and decls, not expressions. Clearly we shouldn't fault, but my first inclination is that the attribute applies to types or decls, not expressions. In that case we should just be issuing an error. I could be convinced otherwise, so if you think we should support passing expressions to this builtin, make your case. jeff
[PATCH] handle expressions in __builtin_has_attribute (PR 88383)
The __builtin_has_attribute function fails with an ICE when
its first argument is an expression such as INDIRECT_REF, or
many others. The code simply assumes it's either a type or
a decl. The attached patch corrects this oversight.
While testing the fix I also noticed the C++ front end expects
the first operand to be a unary expression, which causes most
other kinds of expressions to be rejected. The patch fixes
that as well.
Finally, while testing the fix even more I realized that
the built-in considers the underlying array itself in ARRAY_REF
expressions rather than its type, which leads to inconsistent
results for overaligned arrays (it's the array itself that's
overaligned, not its elements). So I fixed that too and
adjusted the one test designed to verify this.
Tested on x86_64-linux.
Martin
PR c/88383 - ICE calling __builtin_has_attribute on a reference
gcc/c-family/ChangeLog:
PR c/88383
* c-attribs.c (validate_attribute): Handle expressions.
(has_attribute): Handle types referenced by expressions.
Avoid considering array attributes in ARRAY_REF expressions .
gcc/cp/ChangeLog:
PR c/88383
* parser.c (cp_parser_has_attribute_expression): Handle assignment
expressions.
gcc/testsuite/ChangeLog:
PR c/88383
* c-c++-common/builtin-has-attribute-4.c: Adjust expectations.
* c-c++-common/builtin-has-attribute-6.c: New test.
Index: gcc/c-family/c-attribs.c
===
--- gcc/c-family/c-attribs.c (revision 266799)
+++ gcc/c-family/c-attribs.c (working copy)
@@ -3994,8 +3994,10 @@ validate_attribute (location_t atloc, tree oper, t
if (TYPE_P (oper))
tmpdecl = build_decl (atloc, TYPE_DECL, tmpid, oper);
- else
+ else if (DECL_P (oper))
tmpdecl = build_decl (atloc, TREE_CODE (oper), tmpid, TREE_TYPE (oper));
+ else if (EXPR_P (oper))
+tmpdecl = build_decl (atloc, TYPE_DECL, tmpid, TREE_TYPE (oper));
/* Temporarily clear CURRENT_FUNCTION_DECL to make decl_attributes
believe the DECL declared above is at file scope. (See bug 87526.) */
@@ -4040,11 +4042,17 @@ has_attribute (location_t atloc, tree t, tree attr
do
{
/* Determine the array element/member declaration from
- an ARRAY/COMPONENT_REF. */
+ a COMPONENT_REF and an INDIRECT_REF involving a refeence. */
STRIP_NOPS (t);
tree_code code = TREE_CODE (t);
- if (code == ARRAY_REF)
- t = TREE_OPERAND (t, 0);
+ if (code == INDIRECT_REF)
+ {
+ tree op0 = TREE_OPERAND (t, 0);
+ if (TREE_CODE (TREE_TYPE (op0)) == REFERENCE_TYPE)
+ t = op0;
+ else
+ break;
+ }
else if (code == COMPONENT_REF)
t = TREE_OPERAND (t, 1);
else
@@ -4095,7 +4103,8 @@ has_attribute (location_t atloc, tree t, tree attr
}
else
{
- atlist = TYPE_ATTRIBUTES (TREE_TYPE (expr));
+ type = TREE_TYPE (expr);
+ atlist = TYPE_ATTRIBUTES (type);
done = true;
}
Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c (revision 266799)
+++ gcc/cp/parser.c (working copy)
@@ -8480,9 +8480,9 @@ cp_parser_has_attribute_expression (cp_parser *par
cp_parser_parse_definitely (parser);
/* If the type-id production did not work out, then we must be
- looking at the unary-expression production. */
+ looking at an expression. */
if (!oper || oper == error_mark_node)
-oper = cp_parser_unary_expression (parser);
+oper = cp_parser_assignment_expression (parser);
/* Go back to evaluating expressions. */
--cp_unevaluated_operand;
Index: gcc/testsuite/c-c++-common/builtin-has-attribute-4.c
===
--- gcc/testsuite/c-c++-common/builtin-has-attribute-4.c (revision 266799)
+++ gcc/testsuite/c-c++-common/builtin-has-attribute-4.c (working copy)
@@ -151,7 +151,8 @@ void test_packed (struct PackedMember *p)
A (0, gpak[0].c, packed);
A (0, gpak[1].s, packed);
A (1, gpak->a, packed);
- A (1, (*gpak).a[0], packed);
+ /* It's the array that's declared packed but not its elements. */
+ A (0, (*gpak).a[0], packed);
/* The following fails because in C it's represented as
INDIRECT_REF (POINTER_PLUS (NOP_EXPR (ADDR_EXPR (gpak)), ...))
@@ -161,7 +162,8 @@ void test_packed (struct PackedMember *p)
A (0, p->c, packed);
A (0, p->s, packed);
A (1, p->a, packed);
- A (1, p->a[0], packed);
+ /* It's the array that's declared packed but not its elements. */
+ A (0, p->a[0], packed);
/* Similar to the comment above.
A (1, *p->a, packed); */
}
Index: gcc/testsuite/c-c++-common/builtin-has-attribute-6.c
===
--- gcc/testsuite/c-c++-common/builtin-has-attribute-6.c (nonexistent)
+++ gcc/testsuite/c-c++-common/builtin-has-attribute-6.c (working copy)
@@ -0,0 +1,105 @@
+/* PR c/88383 - ICE calling _builtin_has_attribute(r, aligned(N)))
+ on an overaligned reference r
+ {
