Re: [PATCH] add simple attribute introspection
On 11/25/18 7:11 AM, Rainer Orth wrote: Hi Martin, I'm seeing the same on sparc-sun-solaris2.11, plus +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++14 (test for excess errors) +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++17 (test for excess errors) +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++98 (test for excess errors) I see these errors with my sparc-sun-solaris2.11 cross-compiler: /ssd/src/gcc/svn/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:197:3: error: constructor priorities are not supported Apparently the sparc-sun-solaris2.11 target doesn't like ctor and dtor priorities. The error comes from here: [...] The manual doesn't mention anything about this. It makes it sound like ctor/dtor priorities are supported everywhere. They're not, obviously, and we do have an effective-target keyword for this: init_priority. Does Solaris really not support these even with Binutils? If not, I'll adjust the test (and the manual to make it clear the attribute isn't supported everywhere). The test is in gcc/configure.ac (gcc_AC_INITFINI_ARRAY), defined in acinclude.m4. It requires not only target binutils, but also target headers to work; however, you can override it with --enable-initfini-array. You may have pass the same option when configuring binutils at least on Solaris/SPARC (don't remember the details offhand). However, that wasn't the problem in the Solaris 11.4+ results I reported: it *does* support constructor priority even with as/ld and the failures have nothing to do with that issue. Here are the errors/warnings I'm seeing: +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++14 (test for excess errors) +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++17 (test for excess errors) +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++98 (test for excess errors) Excess errors: /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:11:25: error: alignment for 'void faligned_1()' must be at least 4 Right, these are the same issue as those noted by Christophe on aarch64 and arm. I mentioned this issue in my question to the gcc list: https://gcc.gnu.org/ml/gcc/2018-11/msg00127.html I have a patch to let GCC accept less restrictive alignments that I plan to submit (unless someone knows of a reason that makes rejecting this code is necessary). +FAIL: gcc.dg/builtin-has-attribute.c (test for excess errors) Excess errors: /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.dg/builtin-has-attribute.c:12:15: error: size of array 'Assert' is negative +UNRESOLVED: gcc.dg/builtin-has-attribute.c compilation failed to produce executable This one also affects other targets. I explained what causes it in my response here: https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02013.html Fixing it will need a small tweak to the builtin. I'll take care of it this week. The error about missing construcot priority support does occur, however, on Solaris 10 up to 11.3 with ld, in addition to the ones above. As for the test, it would be best to split it into two: one for the bulk that doesn't need constructor priority support and can work everywhere, and another smaller one for the rest, guarded by dg-require-effective-target init_priority. I agree. I'll take care of this as well this week. Martin
Re: [PATCH] add simple attribute introspection
Hi Martin, >> I'm seeing the same on sparc-sun-solaris2.11, plus >> >> +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++14 (test for >> excess errors) >> +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++17 (test for >> excess errors) >> +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++98 (test for >> excess errors) > > I see these errors with my sparc-sun-solaris2.11 cross-compiler: > > /ssd/src/gcc/svn/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:197:3: > error: constructor priorities are not supported > > Apparently the sparc-sun-solaris2.11 target doesn't like ctor and > dtor priorities. The error comes from here: [...] > The manual doesn't mention anything about this. It makes it sound > like ctor/dtor priorities are supported everywhere. They're not, obviously, and we do have an effective-target keyword for this: init_priority. > Does Solaris really not support these even with Binutils? If not, > I'll adjust the test (and the manual to make it clear the attribute > isn't supported everywhere). The test is in gcc/configure.ac (gcc_AC_INITFINI_ARRAY), defined in acinclude.m4. It requires not only target binutils, but also target headers to work; however, you can override it with --enable-initfini-array. You may have pass the same option when configuring binutils at least on Solaris/SPARC (don't remember the details offhand). However, that wasn't the problem in the Solaris 11.4+ results I reported: it *does* support constructor priority even with as/ld and the failures have nothing to do with that issue. Here are the errors/warnings I'm seeing: +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++14 (test for excess errors) +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++17 (test for excess errors) +FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++98 (test for excess errors) Excess errors: /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:11:25: error: alignment for 'void faligned_1()' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:12:25: error: alignment for 'void faligned_2()' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:39:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:40:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:47:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:48:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:50:3: error: size of array 'Assert' is negative /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error: size of array 'Assert' is negative /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:53:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:56:3: error: size of array 'Assert' is negative /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:58:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error: size of array 'Assert' is negative +FAIL: c-c++-common/builtin-has-attribute-3.c -Wc++-compat (test for excess errors) Excess errors: /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:11:25: error: alignment for 'faligned_1' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:12:25: error: alignment for 'faligned_2' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:39:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:40:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:47:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least 4 /vol/gcc/src/hg/trunk/local/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:48:3: error: alignment for '__builtin_has_attribute_tmp.' must be at least
Re: [PATCH] add simple attribute introspection
On 11/22/18 6:09 AM, Rainer Orth wrote:
Hi Christophe,
On Sat, 10 Nov 2018 at 00:43, Martin Sebor wrote:
On 11/09/2018 12:59 PM, Jeff Law wrote:
On 10/31/18 10:27 AM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
With the C++ bits approved I'm still looking for a review/approval
of the remaining changes: the C front end and the shared c-family
handling of the new built-in.
I thought I acked those with just a couple minor doc nits:
I don't see a formal approval for the rest in my Inbox or in
the archive.
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 8ffb0cd..dcf4747 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes
are still necessary.
@cindex @code{flatten} function attribute
Generally, inlining into a function is limited. For a function
marked with
this attribute, every call inside this function is inlined, if possible.
-Whether the function itself is considered for inlining depends on its
size and
-the current inlining parameters.
+Functions declared with attribute @code{noinline} and similar are not
+inlined. Whether the function itself is considered for inlining depends
+on its size and the current inlining parameters.
Guessing this was from another doc patch that I think has already been
approved
Yes. It shouldn't be in the latest patch at the link above.
@@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
@end deftypefn
+@deftypefn {Built-in Function} bool __builtin_has_attribute
(@var{type-or-expression}, @var{attribute})
+The @code{__builtin_has_attribute} function evaluates to an integer
constant
+expression equal to @code{true} if the symbol or type referenced by
+the @var{type-or-expression} argument has been declared with
+the @var{attribute} referenced by the second argument. Neither argument
+is valuated. The @var{type-or-expression} argument is subject to the
same
s/valuated/evaluated/ ?
This should also be fixed in the latest patch at the link above.
Did the implementation change significantly requiring another review
iteration?
I don't think it changed too significantly between the last two
revisions but I don't have a record of anyone having approved
the C FE and the middle-end bits. (Sorry if I missed it.) Other
than this response from you all I see in the archive is this:
https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00606.html
Please let me if the last revision is okay to commit.
Hi,
It seems you committed this yesterday as r266335, and I have noticed
new failures:
on both aarch64 and arm:
FAIL: c-c++-common/builtin-has-attribute-3.c -Wc++-compat (test for
excess errors)
gcc.log says:
Excess errors:
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:11:25: error:
alignment for 'faligned_1' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:12:25: error:
alignment for 'faligned_2' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:39:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:40:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:47:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:48:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:50:3: error:
size of array 'Assert' is negative
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
size of array 'Assert' is negative
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:53:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:56:3: error:
size of array 'Assert' is negative
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:58:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
size of array 'Assert' is negative
on arm only:
gcc.dg/builtin-has-attribute.c (test for excess errors)
gdb.log says:
Excess errors:
/gcc/testsuite/gcc.dg/builtin-has-attribute.c:12:15: error: size of
array 'Assert' is negative
I'm seeing the same on sparc-sun-solaris2.11, plus
+FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++14 (test for excess
errors)
+FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++17 (test for excess
errors)
+FAIL: c-c++-common/builtin-has-attribute-3.c -std=gnu++98 (test for excess
errors)
I see these errors with my sparc
Re: [PATCH] add simple attribute introspection
On 11/22/18 5:28 AM, Christophe Lyon wrote:
On Sat, 10 Nov 2018 at 00:43, Martin Sebor wrote:
On 11/09/2018 12:59 PM, Jeff Law wrote:
On 10/31/18 10:27 AM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
With the C++ bits approved I'm still looking for a review/approval
of the remaining changes: the C front end and the shared c-family
handling of the new built-in.
I thought I acked those with just a couple minor doc nits:
I don't see a formal approval for the rest in my Inbox or in
the archive.
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 8ffb0cd..dcf4747 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes
are still necessary.
@cindex @code{flatten} function attribute
Generally, inlining into a function is limited. For a function
marked with
this attribute, every call inside this function is inlined, if possible.
-Whether the function itself is considered for inlining depends on its
size and
-the current inlining parameters.
+Functions declared with attribute @code{noinline} and similar are not
+inlined. Whether the function itself is considered for inlining depends
+on its size and the current inlining parameters.
Guessing this was from another doc patch that I think has already been
approved
Yes. It shouldn't be in the latest patch at the link above.
@@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
@end deftypefn
+@deftypefn {Built-in Function} bool __builtin_has_attribute
(@var{type-or-expression}, @var{attribute})
+The @code{__builtin_has_attribute} function evaluates to an integer
constant
+expression equal to @code{true} if the symbol or type referenced by
+the @var{type-or-expression} argument has been declared with
+the @var{attribute} referenced by the second argument. Neither argument
+is valuated. The @var{type-or-expression} argument is subject to the
same
s/valuated/evaluated/ ?
This should also be fixed in the latest patch at the link above.
Did the implementation change significantly requiring another review
iteration?
I don't think it changed too significantly between the last two
revisions but I don't have a record of anyone having approved
the C FE and the middle-end bits. (Sorry if I missed it.) Other
than this response from you all I see in the archive is this:
https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00606.html
Please let me if the last revision is okay to commit.
Hi,
It seems you committed this yesterday as r266335, and I have noticed
new failures:
on both aarch64 and arm:
FAIL: c-c++-common/builtin-has-attribute-3.c -Wc++-compat (test for
excess errors)
gcc.log says:
Excess errors:
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:11:25: error:
alignment for 'faligned_1' must be at least 4
Test fails on targets whose default function alignment is 4 (or
any other value greater than 1) because the attribute handler
rejects attribute aligned with less restrictive arguments.
That seems unnecessary to me because less restrictive alignments
are trivially satisfied by more restrictive ones. Clang accepts
the code and I'd say it makes sense for GCC to do the same.
The GNU assembler for aarch64 and sparc-solaris2.11 don't seem
to mind it (though it doesn't look like GCC actually emits .align
directives for small alignments; that may need a separate change
from one to relax the handler).
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:12:25: error:
alignment for 'faligned_2' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:39:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:40:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:47:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:48:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:50:3: error:
size of array 'Assert' is negative
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
size of array 'Assert' is negative
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:53:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:56:3: error:
size of array 'Assert' is negative
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:58:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute
Re: [PATCH] add simple attribute introspection
On Thu, 22 Nov 2018 at 14:09, Rainer Orth wrote:
>
> Hi Christophe,
>
> > On Sat, 10 Nov 2018 at 00:43, Martin Sebor wrote:
> >>
> >> On 11/09/2018 12:59 PM, Jeff Law wrote:
> >> > On 10/31/18 10:27 AM, Martin Sebor wrote:
> >> >> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
> >> >>
> >> >> With the C++ bits approved I'm still looking for a review/approval
> >> >> of the remaining changes: the C front end and the shared c-family
> >> >> handling of the new built-in.
> >> > I thought I acked those with just a couple minor doc nits:
> >>
> >> I don't see a formal approval for the rest in my Inbox or in
> >> the archive.
> >>
> >> >> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> >> >> index 8ffb0cd..dcf4747 100644
> >> >> --- a/gcc/doc/extend.texi
> >> >> +++ b/gcc/doc/extend.texi
> >> >> @@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes
> >> > are still necessary.
> >> >> @cindex @code{flatten} function attribute
> >> >> Generally, inlining into a function is limited. For a function
> >> > marked with
> >> >> this attribute, every call inside this function is inlined, if
> >> >> possible.
> >> >> -Whether the function itself is considered for inlining depends on its
> >> > size and
> >> >> -the current inlining parameters.
> >> >> +Functions declared with attribute @code{noinline} and similar are not
> >> >> +inlined. Whether the function itself is considered for inlining
> >> >> depends
> >> >> +on its size and the current inlining parameters.
> >> > Guessing this was from another doc patch that I think has already been
> >> > approved
> >>
> >> Yes. It shouldn't be in the latest patch at the link above.
> >>
> >> >> @@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
> >> >>
> >> >> @end deftypefn
> >> >>
> >> >> +@deftypefn {Built-in Function} bool __builtin_has_attribute
> >> > (@var{type-or-expression}, @var{attribute})
> >> >> +The @code{__builtin_has_attribute} function evaluates to an integer
> >> > constant
> >> >> +expression equal to @code{true} if the symbol or type referenced by
> >> >> +the @var{type-or-expression} argument has been declared with
> >> >> +the @var{attribute} referenced by the second argument. Neither
> >> >> argument
> >> >> +is valuated. The @var{type-or-expression} argument is subject to the
> >> > same
> >> > s/valuated/evaluated/ ?
> >>
> >> This should also be fixed in the latest patch at the link above.
> >>
> >> > Did the implementation change significantly requiring another review
> >> > iteration?
> >>
> >> I don't think it changed too significantly between the last two
> >> revisions but I don't have a record of anyone having approved
> >> the C FE and the middle-end bits. (Sorry if I missed it.) Other
> >> than this response from you all I see in the archive is this:
> >>
> >>https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00606.html
> >>
> >> Please let me if the last revision is okay to commit.
> >>
> >
> > Hi,
> >
> > It seems you committed this yesterday as r266335, and I have noticed
> > new failures:
> > on both aarch64 and arm:
> > FAIL: c-c++-common/builtin-has-attribute-3.c -Wc++-compat (test for
> > excess errors)
> >
> > gcc.log says:
> > Excess errors:
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:11:25: error:
> > alignment for 'faligned_1' must be at least 4
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:12:25: error:
> > alignment for 'faligned_2' must be at least 4
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:39:3: error:
> > alignment for '__builtin_has_attribute_tmp.' must be at least 4
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:40:3: error:
> > alignment for '__builtin_has_attribute_tmp.' must be at least 4
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:47:3: error:
> > alignment for '__builtin_has_attribute_tmp.' must be at least 4
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:48:3: error:
> > alignment for '__builtin_has_attribute_tmp.' must be at least 4
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:50:3: error:
> > size of array 'Assert' is negative
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
> > alignment for '__builtin_has_attribute_tmp.' must be at least 4
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
> > size of array 'Assert' is negative
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:53:3: error:
> > alignment for '__builtin_has_attribute_tmp.' must be at least 4
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:56:3: error:
> > size of array 'Assert' is negative
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:58:3: error:
> > alignment for '__builtin_has_attribute_tmp.' must be at least 4
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
> > alignment for '__builtin_has_attribute_tmp.' must be at least 4
> > /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
>
Re: [PATCH] add simple attribute introspection
Hi Christophe,
> On Sat, 10 Nov 2018 at 00:43, Martin Sebor wrote:
>>
>> On 11/09/2018 12:59 PM, Jeff Law wrote:
>> > On 10/31/18 10:27 AM, Martin Sebor wrote:
>> >> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
>> >>
>> >> With the C++ bits approved I'm still looking for a review/approval
>> >> of the remaining changes: the C front end and the shared c-family
>> >> handling of the new built-in.
>> > I thought I acked those with just a couple minor doc nits:
>>
>> I don't see a formal approval for the rest in my Inbox or in
>> the archive.
>>
>> >> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>> >> index 8ffb0cd..dcf4747 100644
>> >> --- a/gcc/doc/extend.texi
>> >> +++ b/gcc/doc/extend.texi
>> >> @@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes
>> > are still necessary.
>> >> @cindex @code{flatten} function attribute
>> >> Generally, inlining into a function is limited. For a function
>> > marked with
>> >> this attribute, every call inside this function is inlined, if possible.
>> >> -Whether the function itself is considered for inlining depends on its
>> > size and
>> >> -the current inlining parameters.
>> >> +Functions declared with attribute @code{noinline} and similar are not
>> >> +inlined. Whether the function itself is considered for inlining depends
>> >> +on its size and the current inlining parameters.
>> > Guessing this was from another doc patch that I think has already been
>> > approved
>>
>> Yes. It shouldn't be in the latest patch at the link above.
>>
>> >> @@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
>> >>
>> >> @end deftypefn
>> >>
>> >> +@deftypefn {Built-in Function} bool __builtin_has_attribute
>> > (@var{type-or-expression}, @var{attribute})
>> >> +The @code{__builtin_has_attribute} function evaluates to an integer
>> > constant
>> >> +expression equal to @code{true} if the symbol or type referenced by
>> >> +the @var{type-or-expression} argument has been declared with
>> >> +the @var{attribute} referenced by the second argument. Neither argument
>> >> +is valuated. The @var{type-or-expression} argument is subject to the
>> > same
>> > s/valuated/evaluated/ ?
>>
>> This should also be fixed in the latest patch at the link above.
>>
>> > Did the implementation change significantly requiring another review
>> > iteration?
>>
>> I don't think it changed too significantly between the last two
>> revisions but I don't have a record of anyone having approved
>> the C FE and the middle-end bits. (Sorry if I missed it.) Other
>> than this response from you all I see in the archive is this:
>>
>>https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00606.html
>>
>> Please let me if the last revision is okay to commit.
>>
>
> Hi,
>
> It seems you committed this yesterday as r266335, and I have noticed
> new failures:
> on both aarch64 and arm:
> FAIL: c-c++-common/builtin-has-attribute-3.c -Wc++-compat (test for
> excess errors)
>
> gcc.log says:
> Excess errors:
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:11:25: error:
> alignment for 'faligned_1' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:12:25: error:
> alignment for 'faligned_2' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:39:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:40:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:47:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:48:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:50:3: error:
> size of array 'Assert' is negative
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
> size of array 'Assert' is negative
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:53:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:56:3: error:
> size of array 'Assert' is negative
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:58:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
> alignment for '__builtin_has_attribute_tmp.' must be at least 4
> /gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
> size of array 'Assert' is negative
>
> on arm only:
> gcc.dg/builtin-has-attribute.c (test for excess errors)
> gdb.log says:
> Excess errors:
> /gcc/testsuite/gcc.dg/builtin-has-attribute.c:12:15: error: size of
> array 'Assert' is negative
I'm seeing the same on sparc-sun-solaris2.
Re: [PATCH] add simple attribute introspection
On Sat, 10 Nov 2018 at 00:43, Martin Sebor wrote:
>
> On 11/09/2018 12:59 PM, Jeff Law wrote:
> > On 10/31/18 10:27 AM, Martin Sebor wrote:
> >> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
> >>
> >> With the C++ bits approved I'm still looking for a review/approval
> >> of the remaining changes: the C front end and the shared c-family
> >> handling of the new built-in.
> > I thought I acked those with just a couple minor doc nits:
>
> I don't see a formal approval for the rest in my Inbox or in
> the archive.
>
> >> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> >> index 8ffb0cd..dcf4747 100644
> >> --- a/gcc/doc/extend.texi
> >> +++ b/gcc/doc/extend.texi
> >> @@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes
> > are still necessary.
> >> @cindex @code{flatten} function attribute
> >> Generally, inlining into a function is limited. For a function
> > marked with
> >> this attribute, every call inside this function is inlined, if possible.
> >> -Whether the function itself is considered for inlining depends on its
> > size and
> >> -the current inlining parameters.
> >> +Functions declared with attribute @code{noinline} and similar are not
> >> +inlined. Whether the function itself is considered for inlining depends
> >> +on its size and the current inlining parameters.
> > Guessing this was from another doc patch that I think has already been
> > approved
>
> Yes. It shouldn't be in the latest patch at the link above.
>
> >> @@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
> >>
> >> @end deftypefn
> >>
> >> +@deftypefn {Built-in Function} bool __builtin_has_attribute
> > (@var{type-or-expression}, @var{attribute})
> >> +The @code{__builtin_has_attribute} function evaluates to an integer
> > constant
> >> +expression equal to @code{true} if the symbol or type referenced by
> >> +the @var{type-or-expression} argument has been declared with
> >> +the @var{attribute} referenced by the second argument. Neither argument
> >> +is valuated. The @var{type-or-expression} argument is subject to the
> > same
> > s/valuated/evaluated/ ?
>
> This should also be fixed in the latest patch at the link above.
>
> > Did the implementation change significantly requiring another review
> > iteration?
>
> I don't think it changed too significantly between the last two
> revisions but I don't have a record of anyone having approved
> the C FE and the middle-end bits. (Sorry if I missed it.) Other
> than this response from you all I see in the archive is this:
>
>https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00606.html
>
> Please let me if the last revision is okay to commit.
>
Hi,
It seems you committed this yesterday as r266335, and I have noticed
new failures:
on both aarch64 and arm:
FAIL: c-c++-common/builtin-has-attribute-3.c -Wc++-compat (test for
excess errors)
gcc.log says:
Excess errors:
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:11:25: error:
alignment for 'faligned_1' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:12:25: error:
alignment for 'faligned_2' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:39:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:40:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:47:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:48:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:50:3: error:
size of array 'Assert' is negative
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:52:3: error:
size of array 'Assert' is negative
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:53:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:56:3: error:
size of array 'Assert' is negative
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:58:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
alignment for '__builtin_has_attribute_tmp.' must be at least 4
/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c:59:3: error:
size of array 'Assert' is negative
on arm only:
gcc.dg/builtin-has-attribute.c (test for excess errors)
gdb.log says:
Excess errors:
/gcc/testsuite/gcc.dg/builtin-has-attribute.c:12:15: error: size of
array 'Assert' is negative
Christophe
> Thanks
> Martin
Re: PING [PATCH] add simple attribute introspection
On 11/15/18 8:06 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
> (Still looking for an approval.)
>
> On 11/09/2018 04:43 PM, Martin Sebor wrote:
>> On 11/09/2018 12:59 PM, Jeff Law wrote:
>>> On 10/31/18 10:27 AM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
With the C++ bits approved I'm still looking for a review/approval
of the remaining changes: the C front end and the shared c-family
handling of the new built-in.
>>> I thought I acked those with just a couple minor doc nits:
>>
>> I don't see a formal approval for the rest in my Inbox or in
>> the archive.
>>
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 8ffb0cd..dcf4747 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes
>>> are still necessary.
@cindex @code{flatten} function attribute
Generally, inlining into a function is limited. For a function
>>> marked with
this attribute, every call inside this function is inlined, if
possible.
-Whether the function itself is considered for inlining depends on its
>>> size and
-the current inlining parameters.
+Functions declared with attribute @code{noinline} and similar are not
+inlined. Whether the function itself is considered for inlining
depends
+on its size and the current inlining parameters.
>>> Guessing this was from another doc patch that I think has already been
>>> approved
>>
>> Yes. It shouldn't be in the latest patch at the link above.
>>
@@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
@end deftypefn
+@deftypefn {Built-in Function} bool __builtin_has_attribute
>>> (@var{type-or-expression}, @var{attribute})
+The @code{__builtin_has_attribute} function evaluates to an integer
>>> constant
+expression equal to @code{true} if the symbol or type referenced by
+the @var{type-or-expression} argument has been declared with
+the @var{attribute} referenced by the second argument. Neither
argument
+is valuated. The @var{type-or-expression} argument is subject to the
>>> same
>>> s/valuated/evaluated/ ?
>>
>> This should also be fixed in the latest patch at the link above.
>>
>>> Did the implementation change significantly requiring another review
>>> iteration?
>>
>> I don't think it changed too significantly between the last two
>> revisions but I don't have a record of anyone having approved
>> the C FE and the middle-end bits. (Sorry if I missed it.) Other
>> than this response from you all I see in the archive is this:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00606.html
>>
>> Please let me if the last revision is okay to commit.
Yes. It's fine to commit.
jeff
PING [PATCH] add simple attribute introspection
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
(Still looking for an approval.)
On 11/09/2018 04:43 PM, Martin Sebor wrote:
On 11/09/2018 12:59 PM, Jeff Law wrote:
On 10/31/18 10:27 AM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
With the C++ bits approved I'm still looking for a review/approval
of the remaining changes: the C front end and the shared c-family
handling of the new built-in.
I thought I acked those with just a couple minor doc nits:
I don't see a formal approval for the rest in my Inbox or in
the archive.
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 8ffb0cd..dcf4747 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes
are still necessary.
@cindex @code{flatten} function attribute
Generally, inlining into a function is limited. For a function
marked with
this attribute, every call inside this function is inlined, if
possible.
-Whether the function itself is considered for inlining depends on its
size and
-the current inlining parameters.
+Functions declared with attribute @code{noinline} and similar are not
+inlined. Whether the function itself is considered for inlining
depends
+on its size and the current inlining parameters.
Guessing this was from another doc patch that I think has already been
approved
Yes. It shouldn't be in the latest patch at the link above.
@@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
@end deftypefn
+@deftypefn {Built-in Function} bool __builtin_has_attribute
(@var{type-or-expression}, @var{attribute})
+The @code{__builtin_has_attribute} function evaluates to an integer
constant
+expression equal to @code{true} if the symbol or type referenced by
+the @var{type-or-expression} argument has been declared with
+the @var{attribute} referenced by the second argument. Neither
argument
+is valuated. The @var{type-or-expression} argument is subject to the
same
s/valuated/evaluated/ ?
This should also be fixed in the latest patch at the link above.
Did the implementation change significantly requiring another review
iteration?
I don't think it changed too significantly between the last two
revisions but I don't have a record of anyone having approved
the C FE and the middle-end bits. (Sorry if I missed it.) Other
than this response from you all I see in the archive is this:
https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00606.html
Please let me if the last revision is okay to commit.
Thanks
Martin
Re: [PATCH] add simple attribute introspection
On 11/09/2018 12:59 PM, Jeff Law wrote:
On 10/31/18 10:27 AM, Martin Sebor wrote:
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
With the C++ bits approved I'm still looking for a review/approval
of the remaining changes: the C front end and the shared c-family
handling of the new built-in.
I thought I acked those with just a couple minor doc nits:
I don't see a formal approval for the rest in my Inbox or in
the archive.
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 8ffb0cd..dcf4747 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes
are still necessary.
@cindex @code{flatten} function attribute
Generally, inlining into a function is limited. For a function
marked with
this attribute, every call inside this function is inlined, if possible.
-Whether the function itself is considered for inlining depends on its
size and
-the current inlining parameters.
+Functions declared with attribute @code{noinline} and similar are not
+inlined. Whether the function itself is considered for inlining depends
+on its size and the current inlining parameters.
Guessing this was from another doc patch that I think has already been
approved
Yes. It shouldn't be in the latest patch at the link above.
@@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
@end deftypefn
+@deftypefn {Built-in Function} bool __builtin_has_attribute
(@var{type-or-expression}, @var{attribute})
+The @code{__builtin_has_attribute} function evaluates to an integer
constant
+expression equal to @code{true} if the symbol or type referenced by
+the @var{type-or-expression} argument has been declared with
+the @var{attribute} referenced by the second argument. Neither argument
+is valuated. The @var{type-or-expression} argument is subject to the
same
s/valuated/evaluated/ ?
This should also be fixed in the latest patch at the link above.
Did the implementation change significantly requiring another review
iteration?
I don't think it changed too significantly between the last two
revisions but I don't have a record of anyone having approved
the C FE and the middle-end bits. (Sorry if I missed it.) Other
than this response from you all I see in the archive is this:
https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00606.html
Please let me if the last revision is okay to commit.
Thanks
Martin
Re: [PATCH] add simple attribute introspection
On 10/31/18 10:27 AM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
>
> With the C++ bits approved I'm still looking for a review/approval
> of the remaining changes: the C front end and the shared c-family
> handling of the new built-in.
I thought I acked those with just a couple minor doc nits:
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 8ffb0cd..dcf4747 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes
are still necessary.
> @cindex @code{flatten} function attribute
> Generally, inlining into a function is limited. For a function
marked with
> this attribute, every call inside this function is inlined, if possible.
> -Whether the function itself is considered for inlining depends on its
size and
> -the current inlining parameters.
> +Functions declared with attribute @code{noinline} and similar are not
> +inlined. Whether the function itself is considered for inlining depends
> +on its size and the current inlining parameters.
Guessing this was from another doc patch that I think has already been
approved
> @@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
>
> @end deftypefn
>
> +@deftypefn {Built-in Function} bool __builtin_has_attribute
(@var{type-or-expression}, @var{attribute})
> +The @code{__builtin_has_attribute} function evaluates to an integer
constant
> +expression equal to @code{true} if the symbol or type referenced by
> +the @var{type-or-expression} argument has been declared with
> +the @var{attribute} referenced by the second argument. Neither argument
> +is valuated. The @var{type-or-expression} argument is subject to the
same
s/valuated/evaluated/ ?
Did the implementation change significantly requiring another review
iteration?
jeff
Re: [PATCH] add simple attribute introspection
Ping: https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01473.html
With the C++ bits approved I'm still looking for a review/approval
of the remaining changes: the C front end and the shared c-family
handling of the new built-in.
On 10/23/2018 04:08 PM, Martin Sebor wrote:
On 10/22/2018 04:08 PM, Jason Merrill wrote:
On 10/13/18 8:19 PM, Martin Sebor wrote:
+ oper = cp_parser_type_id (parser);
+ parser->in_type_id_in_expr_p = saved_in_type_id_in_expr_p;
+
+ if (cp_parser_parse_definitely (parser))
+{
+ /* If all went well, set OPER to the type. */
+ cp_decl_specifier_seq decl_specs;
+
+ /* Build a trivial decl-specifier-seq. */
+ clear_decl_specs (&decl_specs);
+ decl_specs.type = oper;
+
+ /* Call grokdeclarator to figure out what type this is. */
+ oper = grokdeclarator (NULL,
+ &decl_specs,
+ TYPENAME,
+ /*initialized=*/0,
+ /*attrlist=*/NULL);
+}
Doesn't grokdeclarator here give you back the same type you already had
from cp_parser_type_id? The latter already calls grokdeclarator.
I don't know why cp_parser_sizeof_operand does this, either. Try
removing it from both places?
You're right, the call in cp_parser_has_attribute_expression
was unnecessary. cp_parser_sizeof_operand still needs it.
+ /* Consume the comma if it's there. */
+ if (!cp_parser_require (parser, CPP_COMMA, RT_COMMA))
+{
+ parens.require_close (parser);
I think you want cp_parser_skip_to_closing_parenthesis for error
recovery, rather than require_close.
Thanks, the error messages look slightly better that way (there
are fewer of them), although still not as good as in C or other
compilers in some cases.
+ if (tree attr = cp_parser_gnu_attribute_list (parser,
/*exactly_one=*/true))
+{
+ if (oper != error_mark_node)
+{
+ /* Fold constant expressions used in attributes first. */
+ cp_check_const_attributes (attr);
+
+ /* Finally, see if OPER has been declared with ATTR. */
+ ret = has_attribute (atloc, oper, attr, default_conversion);
+}
+}
+ else
+{
+ error_at (atloc, "expected identifier");
+ cp_parser_skip_to_closing_parenthesis (parser, true, false,
false);
+}
+
+ parens.require_close (parser);
I think the require_close should be in the valid case, since *skip*
already consumes a closing paren.
Ah, I need to make it consume the paren by passing true as the last
argument. With that it works.
+is valuated. The @var{type-or-expression} argument is subject to the
same
evaluated
Thanks for the review.
Attached is an updated patch with the fixes above.
Martin
Re: [PATCH] add simple attribute introspection
On 10/23/18 6:08 PM, Martin Sebor wrote:
+ if (cp_parser_parse_definitely (parser))
+{
+ /* If all went well, set OPER to the type. */
+ cp_decl_specifier_seq decl_specs;
+
+ /* Build a trivial decl-specifier-seq. */
+ clear_decl_specs (&decl_specs);
+ decl_specs.type = oper;
+}
+
+ /* If the type-id production did not work out, then we must be
+ looking at the unary-expression production. */
+ if (!oper || oper == error_mark_node)
+oper = cp_parser_unary_expression (parser);
The decl_specs stuff is still unneeded; I think all this can be
simplified to
if (!cp_parser_parse_definitely (parser))
oper = cp_parser_unary_expression (parser);
The C++ bits are OK with that change.
You probably want a follow-on patch to handle
template-argument-dependent cases, like when the alignment argument
depends on a template parameter.
Jason
Re: [PATCH] add simple attribute introspection
On 10/22/2018 04:08 PM, Jason Merrill wrote:
On 10/13/18 8:19 PM, Martin Sebor wrote:
+ oper = cp_parser_type_id (parser);
+ parser->in_type_id_in_expr_p = saved_in_type_id_in_expr_p;
+
+ if (cp_parser_parse_definitely (parser))
+{
+ /* If all went well, set OPER to the type. */
+ cp_decl_specifier_seq decl_specs;
+
+ /* Build a trivial decl-specifier-seq. */
+ clear_decl_specs (&decl_specs);
+ decl_specs.type = oper;
+
+ /* Call grokdeclarator to figure out what type this is. */
+ oper = grokdeclarator (NULL,
+ &decl_specs,
+ TYPENAME,
+ /*initialized=*/0,
+ /*attrlist=*/NULL);
+}
Doesn't grokdeclarator here give you back the same type you already had
from cp_parser_type_id? The latter already calls grokdeclarator.
I don't know why cp_parser_sizeof_operand does this, either. Try
removing it from both places?
You're right, the call in cp_parser_has_attribute_expression
was unnecessary. cp_parser_sizeof_operand still needs it.
+ /* Consume the comma if it's there. */
+ if (!cp_parser_require (parser, CPP_COMMA, RT_COMMA))
+{
+ parens.require_close (parser);
I think you want cp_parser_skip_to_closing_parenthesis for error
recovery, rather than require_close.
Thanks, the error messages look slightly better that way (there
are fewer of them), although still not as good as in C or other
compilers in some cases.
+ if (tree attr = cp_parser_gnu_attribute_list (parser,
/*exactly_one=*/true))
+{
+ if (oper != error_mark_node)
+{
+ /* Fold constant expressions used in attributes first. */
+ cp_check_const_attributes (attr);
+
+ /* Finally, see if OPER has been declared with ATTR. */
+ ret = has_attribute (atloc, oper, attr, default_conversion);
+}
+}
+ else
+{
+ error_at (atloc, "expected identifier");
+ cp_parser_skip_to_closing_parenthesis (parser, true, false,
false);
+}
+
+ parens.require_close (parser);
I think the require_close should be in the valid case, since *skip*
already consumes a closing paren.
Ah, I need to make it consume the paren by passing true as the last
argument. With that it works.
+is valuated. The @var{type-or-expression} argument is subject to the
same
evaluated
Thanks for the review.
Attached is an updated patch with the fixes above.
Martin
gcc/c/ChangeLog:
* c-parser.c (c_parser_has_attribute_expression): New function.
(c_parser_attribute): New function.
(c_parser_attributes): Move code into c_parser_attribute.
(c_parser_unary_expression): Handle RID_HAS_ATTRIBUTE_EXPRESSION.
gcc/c-family/ChangeLog:
* c-attribs.c (type_for_vector_size): New function.
(type_valid_for_vector_size): Same.
(handle_vector_size_attribute): Move code to the functions above
and call them.
(validate_attribute, has_attribute): New functions.
* c-common.h (has_attribute): Declare.
(rid): Add RID_HAS_ATTRIBUTE_EXPRESSION.
* c-common.c (c_common_resword): Same.
gcc/cp/ChangeLog:
* cp-tree.h (cp_check_const_attributes): Declare.
* decl2.c (cp_check_const_attributes): Declare extern.
* parser.c (cp_parser_has_attribute_expression): New function.
(cp_parser_unary_expression): Handle RID_HAS_ATTRIBUTE_EXPRESSION.
(cp_parser_gnu_attribute_list): Add argument.
gcc/ChangeLog:
* doc/extend.texi (Other Builtins): Add __builtin_has_attribute.
gcc/testsuite/ChangeLog:
* c-c++-common/builtin-has-attribute-2.c: New test.
* c-c++-common/builtin-has-attribute-3.c: New test.
* c-c++-common/builtin-has-attribute-4.c: New test.
* c-c++-common/builtin-has-attribute.c: New test.
* gcc.dg/builtin-has-attribute.c: New test.
* gcc/testsuite/gcc.target/i386/builtin-has-attribute.c: New test.
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 3a88766..c0a1bb5 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -3128,34 +3128,11 @@ handle_deprecated_attribute (tree *node, tree name,
return NULL_TREE;
}
-/* Handle a "vector_size" attribute; arguments as in
- struct attribute_spec.handler. */
-
+/* Return the "base" type from TYPE that is suitable to apply attribute
+ vector_size to by stripping arrays, function types, etc. */
static tree
-handle_vector_size_attribute (tree *node, tree name, tree args,
- int ARG_UNUSED (flags),
- bool *no_add_attrs)
+type_for_vector_size (tree type)
{
- unsigned HOST_WIDE_INT vecsize, nunits;
- machine_mode orig_mode;
- tree type = *node, new_type, size;
-
- *no_add_attrs = true;
-
- size = TREE_VALUE (args);
- if (size && TREE_CODE (size) != IDENTIFIER_NODE
- && TREE_CODE (size) != FUNCTION_DECL)
-size = default_conversion (size);
-
- if (!tree_fits_uhwi_p (size))
-{
- warning (OPT_Wattributes, "%qE attribute ignored", name);
- return NULL_TREE;
-}
-
- /* Get the vector size (in bytes). */
- vecsize = tree_to_uhwi (size);
-
/* We need to provide
Re: [PATCH] add simple attribute introspection
On 10/13/18 8:19 PM, Martin Sebor wrote:
+ oper = cp_parser_type_id (parser);
+ parser->in_type_id_in_expr_p = saved_in_type_id_in_expr_p;
+
+ if (cp_parser_parse_definitely (parser))
+{
+ /* If all went well, set OPER to the type. */
+ cp_decl_specifier_seq decl_specs;
+
+ /* Build a trivial decl-specifier-seq. */
+ clear_decl_specs (&decl_specs);
+ decl_specs.type = oper;
+
+ /* Call grokdeclarator to figure out what type this is. */
+ oper = grokdeclarator (NULL,
+&decl_specs,
+TYPENAME,
+/*initialized=*/0,
+/*attrlist=*/NULL);
+}
Doesn't grokdeclarator here give you back the same type you already had
from cp_parser_type_id? The latter already calls grokdeclarator.
I don't know why cp_parser_sizeof_operand does this, either. Try
removing it from both places?
+ /* Consume the comma if it's there. */
+ if (!cp_parser_require (parser, CPP_COMMA, RT_COMMA))
+{
+ parens.require_close (parser);
I think you want cp_parser_skip_to_closing_parenthesis for error
recovery, rather than require_close.
+ if (tree attr = cp_parser_gnu_attribute_list (parser, /*exactly_one=*/true))
+{
+ if (oper != error_mark_node)
+ {
+ /* Fold constant expressions used in attributes first. */
+ cp_check_const_attributes (attr);
+
+ /* Finally, see if OPER has been declared with ATTR. */
+ ret = has_attribute (atloc, oper, attr, default_conversion);
+ }
+}
+ else
+{
+ error_at (atloc, "expected identifier");
+ cp_parser_skip_to_closing_parenthesis (parser, true, false, false);
+}
+
+ parens.require_close (parser);
I think the require_close should be in the valid case, since *skip*
already consumes a closing paren.
+is valuated. The @var{type-or-expression} argument is subject to the same
evaluated
Jason
Re: [PATCH] add simple attribute introspection
Jason,
Do you have any suggestions for the C++ parts or are they good
enough to commit (with the expectation that I'll add template
handling later)?
The last patch is here:
https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00811.html
Martin
On 10/16/2018 05:19 PM, Jeff Law wrote:
On 10/13/18 6:19 PM, Martin Sebor wrote:
Attached is an updated/enhanced patch with many more tests
and the suggested documentation tweak. It also restores
the handling of empty attributes that the first revision
inadvertently removed from the C parser.
The tests are much more comprehensive now but still not
exhaustive. I have added warning for the mode attribute
that cannot be supported. Enumerator attributes aren't
detected in C because they are folded to constants before
they reach the built-in, and label attributes aren't handled
yet either in C or in C++. Supporting those will take a minor
enhancement. I haven only added handful of test cases for
x86_64 target attributes, The built-in is not exercised
for any other target yet. I don't expect any surprises
there. Either it will work or (where the attributes aren't
hanging off a node) it will return false. Supporting those
will have to wait until the later (I think the best way is
to add a callback to struct attribute_spec to let each back
end query a node for the properties unique to such attributes
analogously to attribute vector_size).
I haven't done any work on supporting templates. I would
like to but I don't expect to be able to get it done before
stage 1 is over (I have another feature I need to finish,
the one that prompted this work to begin with). I think
the new built-in is quite useful even without template
support.
I've kept the name __builtin_has_attribute: it is close to
the __has_attribute macro, and I think that's fine because
the built-in's purpose is very close to that of the macro.
Martin
gcc-builtin-has-attribute.diff
gcc/c/ChangeLog:
* c-parser.c (c_parser_has_attribute_expression): New function.
(c_parser_attribute): New function.
(c_parser_attributes): Move code into c_parser_attribute.
(c_parser_unary_expression): Handle RID_HAS_ATTRIBUTE_EXPRESSION.
gcc/c-family/ChangeLog:
* c-attribs.c (type_for_vector_size): New function.
(type_valid_for_vector_size): Same.
(handle_vector_size_attribute): Move code to the functions above
and call them.
(validate_attribute, has_attribute): New functions.
* c-common.h (has_attribute): Declare.
(rid): Add RID_HAS_ATTRIBUTE_EXPRESSION.
* c-common.c (c_common_resword): Same.
gcc/cp/ChangeLog:
* cp-tree.h (cp_check_const_attributes): Declare.
* decl2.c (cp_check_const_attributes): Declare extern.
* parser.c (cp_parser_has_attribute_expression): New function.
(cp_parser_unary_expression): Handle RID_HAS_ATTRIBUTE_EXPRESSION.
(cp_parser_gnu_attribute_list): Add argument.
gcc/ChangeLog:
* doc/extend.texi (Other Builtins): Add __builtin_has_attribute.
gcc/testsuite/ChangeLog:
* c-c++-common/builtin-has-attribute-2.c: New test.
* c-c++-common/builtin-has-attribute-3.c: New test.
* c-c++-common/builtin-has-attribute-4.c: New test.
* c-c++-common/builtin-has-attribute.c: New test.
* gcc.dg/builtin-has-attribute.c: New test.
* gcc/testsuite/gcc.target/i386/builtin-has-attribute.c: New test.
Generally looks OK except for a nit mentioned below. I don't mind
iterating a bit on the details here over time. Give Jason a few days to
chime in on the C++ side.
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 8ffb0cd..dcf4747 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes are still
necessary.
@cindex @code{flatten} function attribute
Generally, inlining into a function is limited. For a function marked with
this attribute, every call inside this function is inlined, if possible.
-Whether the function itself is considered for inlining depends on its size and
-the current inlining parameters.
+Functions declared with attribute @code{noinline} and similar are not
+inlined. Whether the function itself is considered for inlining depends
+on its size and the current inlining parameters.
Guessing this was from another doc patch that I think has already been
approved :-)
@@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
@end deftypefn
+@deftypefn {Built-in Function} bool __builtin_has_attribute
(@var{type-or-expression}, @var{attribute})
+The @code{__builtin_has_attribute} function evaluates to an integer constant
+expression equal to @code{true} if the symbol or type referenced by
+the @var{type-or-expression} argument has been declared with
+the @var{attribute} referenced by the second argument. Neither argument
+is valuated. The @var{type-or-expression} argument is subject to the same
s/valuated/
Re: [PATCH] add simple attribute introspection
On 10/13/18, Martin Sebor wrote: > Attached is an updated/enhanced patch with many more tests > and the suggested documentation tweak. It also restores > the handling of empty attributes that the first revision > inadvertently removed from the C parser. > > The tests are much more comprehensive now but still not > exhaustive. I have added warning for the mode attribute > that cannot be supported. Enumerator attributes aren't > detected in C because they are folded to constants before > they reach the built-in, and label attributes aren't handled > yet either in C or in C++. Supporting those will take a minor > enhancement. I haven only added handful of test cases for > x86_64 target attributes, The built-in is not exercised > for any other target yet. I don't expect any surprises > there. Either it will work or (where the attributes aren't > hanging off a node) it will return false. Supporting those > will have to wait until the later (I think the best way is > to add a callback to struct attribute_spec to let each back > end query a node for the properties unique to such attributes > analogously to attribute vector_size). > > I haven't done any work on supporting templates. I would > like to but I don't expect to be able to get it done before > stage 1 is over (I have another feature I need to finish, > the one that prompted this work to begin with). I think > the new built-in is quite useful even without template > support. > > I've kept the name __builtin_has_attribute: it is close to > the __has_attribute macro, and I think that's fine because > the built-in's purpose is very close to that of the macro. If you're going to keep the name, could you at least explain the difference between the two in the documentation? > > Martin > > On 10/11/2018 08:54 AM, Martin Sebor wrote: >> On 10/11/2018 06:04 AM, Joseph Myers wrote: >>> On Thu, 11 Oct 2018, Martin Sebor wrote: >>> The attached patch introduces a built-in function called __builtin_has_attribute that makes some of this possible. See the documentation and tests for details. >>> >>> I see nothing in the documentation about handling of equivalent forms of >>> an attribute - for example, specifying __aligned__ in the attribute but >>> aligned in __builtin_has_attribute, or vice versa. I'd expect that to >>> be >>> documented to work (both of those should return true), with associated >>> tests. (And likewise the semantics should allow for a format attribute >>> using printf in one place and __printf__ in the other, for example, or >>> the >>> same constant argument represented with different expressions.) >> >> Yes, it occurred to me belatedly that I should add a test for those >> as well. I can also mention it in the documentation, although I'd >> have thought it would be implicit in how attributes work in general. >> (Or are there some differences between the underscored forms and >> the one without it)? >> >>> >>> What are the semantics of __builtin_has_attribute for attributes that >>> can't be tested for? (E.g. the mode attribute, which ends up >>> resulting in >>> some existing type with the required mode being used, so there's nothing >>> to indicate the attribute was originally used to declare things.) >> >> With a few exceptions (like aligned) the built-in returns false >> for attributes that aren't attached to a node. I haven't exercised >> nearly all the attributes yet, and this one could very well be among >> those that aren't and perhaps can't be handled. I suspect some >> target attributes might be in the same group. If there's no way >> to tell it should probably be documented as a limitation of >> the function, maybe also under the attribute itself that can't >> be detected. Alternatively, the built-in return type could be >> changed to a tri-state: "don't know," false, true. Can you >> think of a better solution? >> >> Martin > >
Re: [PATCH] add simple attribute introspection
On 10/13/18 6:19 PM, Martin Sebor wrote:
> Attached is an updated/enhanced patch with many more tests
> and the suggested documentation tweak. It also restores
> the handling of empty attributes that the first revision
> inadvertently removed from the C parser.
>
> The tests are much more comprehensive now but still not
> exhaustive. I have added warning for the mode attribute
> that cannot be supported. Enumerator attributes aren't
> detected in C because they are folded to constants before
> they reach the built-in, and label attributes aren't handled
> yet either in C or in C++. Supporting those will take a minor
> enhancement. I haven only added handful of test cases for
> x86_64 target attributes, The built-in is not exercised
> for any other target yet. I don't expect any surprises
> there. Either it will work or (where the attributes aren't
> hanging off a node) it will return false. Supporting those
> will have to wait until the later (I think the best way is
> to add a callback to struct attribute_spec to let each back
> end query a node for the properties unique to such attributes
> analogously to attribute vector_size).
>
> I haven't done any work on supporting templates. I would
> like to but I don't expect to be able to get it done before
> stage 1 is over (I have another feature I need to finish,
> the one that prompted this work to begin with). I think
> the new built-in is quite useful even without template
> support.
>
> I've kept the name __builtin_has_attribute: it is close to
> the __has_attribute macro, and I think that's fine because
> the built-in's purpose is very close to that of the macro.
>
> Martin
>
>
>
> gcc-builtin-has-attribute.diff
>
> gcc/c/ChangeLog:
>
> * c-parser.c (c_parser_has_attribute_expression): New function.
> (c_parser_attribute): New function.
> (c_parser_attributes): Move code into c_parser_attribute.
> (c_parser_unary_expression): Handle RID_HAS_ATTRIBUTE_EXPRESSION.
>
> gcc/c-family/ChangeLog:
>
> * c-attribs.c (type_for_vector_size): New function.
> (type_valid_for_vector_size): Same.
> (handle_vector_size_attribute): Move code to the functions above
> and call them.
> (validate_attribute, has_attribute): New functions.
> * c-common.h (has_attribute): Declare.
> (rid): Add RID_HAS_ATTRIBUTE_EXPRESSION.
> * c-common.c (c_common_resword): Same.
>
> gcc/cp/ChangeLog:
>
> * cp-tree.h (cp_check_const_attributes): Declare.
> * decl2.c (cp_check_const_attributes): Declare extern.
> * parser.c (cp_parser_has_attribute_expression): New function.
> (cp_parser_unary_expression): Handle RID_HAS_ATTRIBUTE_EXPRESSION.
> (cp_parser_gnu_attribute_list): Add argument.
>
> gcc/ChangeLog:
>
> * doc/extend.texi (Other Builtins): Add __builtin_has_attribute.
>
> gcc/testsuite/ChangeLog:
>
> * c-c++-common/builtin-has-attribute-2.c: New test.
> * c-c++-common/builtin-has-attribute-3.c: New test.
> * c-c++-common/builtin-has-attribute-4.c: New test.
> * c-c++-common/builtin-has-attribute.c: New test.
> * gcc.dg/builtin-has-attribute.c: New test.
> * gcc/testsuite/gcc.target/i386/builtin-has-attribute.c: New test.
>
Generally looks OK except for a nit mentioned below. I don't mind
iterating a bit on the details here over time. Give Jason a few days to
chime in on the C++ side.
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 8ffb0cd..dcf4747 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -2649,8 +2649,9 @@ explicit @code{externally_visible} attributes are still
> necessary.
> @cindex @code{flatten} function attribute
> Generally, inlining into a function is limited. For a function marked with
> this attribute, every call inside this function is inlined, if possible.
> -Whether the function itself is considered for inlining depends on its size
> and
> -the current inlining parameters.
> +Functions declared with attribute @code{noinline} and similar are not
> +inlined. Whether the function itself is considered for inlining depends
> +on its size and the current inlining parameters.
Guessing this was from another doc patch that I think has already been
approved :-)
> @@ -11726,6 +11728,33 @@ check its compatibility with @var{size}.
>
> @end deftypefn
>
> +@deftypefn {Built-in Function} bool __builtin_has_attribute
> (@var{type-or-expression}, @var{attribute})
> +The @code{__builtin_has_attribute} function evaluates to an integer constant
> +expression equal to @code{true} if the symbol or type referenced by
> +the @var{type-or-expression} argument has been declared with
> +the @var{attribute} referenced by the second argument. Neither argument
> +is valuated. The @var{type-or-expression} argument is subject to the same
s/valuated/evaluated/ ?
Re: [PATCH] add simple attribute introspection
Attached is an updated/enhanced patch with many more tests and the suggested documentation tweak. It also restores the handling of empty attributes that the first revision inadvertently removed from the C parser. The tests are much more comprehensive now but still not exhaustive. I have added warning for the mode attribute that cannot be supported. Enumerator attributes aren't detected in C because they are folded to constants before they reach the built-in, and label attributes aren't handled yet either in C or in C++. Supporting those will take a minor enhancement. I haven only added handful of test cases for x86_64 target attributes, The built-in is not exercised for any other target yet. I don't expect any surprises there. Either it will work or (where the attributes aren't hanging off a node) it will return false. Supporting those will have to wait until the later (I think the best way is to add a callback to struct attribute_spec to let each back end query a node for the properties unique to such attributes analogously to attribute vector_size). I haven't done any work on supporting templates. I would like to but I don't expect to be able to get it done before stage 1 is over (I have another feature I need to finish, the one that prompted this work to begin with). I think the new built-in is quite useful even without template support. I've kept the name __builtin_has_attribute: it is close to the __has_attribute macro, and I think that's fine because the built-in's purpose is very close to that of the macro. Martin On 10/11/2018 08:54 AM, Martin Sebor wrote: On 10/11/2018 06:04 AM, Joseph Myers wrote: On Thu, 11 Oct 2018, Martin Sebor wrote: The attached patch introduces a built-in function called __builtin_has_attribute that makes some of this possible. See the documentation and tests for details. I see nothing in the documentation about handling of equivalent forms of an attribute - for example, specifying __aligned__ in the attribute but aligned in __builtin_has_attribute, or vice versa. I'd expect that to be documented to work (both of those should return true), with associated tests. (And likewise the semantics should allow for a format attribute using printf in one place and __printf__ in the other, for example, or the same constant argument represented with different expressions.) Yes, it occurred to me belatedly that I should add a test for those as well. I can also mention it in the documentation, although I'd have thought it would be implicit in how attributes work in general. (Or are there some differences between the underscored forms and the one without it)? What are the semantics of __builtin_has_attribute for attributes that can't be tested for? (E.g. the mode attribute, which ends up resulting in some existing type with the required mode being used, so there's nothing to indicate the attribute was originally used to declare things.) With a few exceptions (like aligned) the built-in returns false for attributes that aren't attached to a node. I haven't exercised nearly all the attributes yet, and this one could very well be among those that aren't and perhaps can't be handled. I suspect some target attributes might be in the same group. If there's no way to tell it should probably be documented as a limitation of the function, maybe also under the attribute itself that can't be detected. Alternatively, the built-in return type could be changed to a tri-state: "don't know," false, true. Can you think of a better solution? Martin gcc/c/ChangeLog: * c-parser.c (c_parser_has_attribute_expression): New function. (c_parser_attribute): New function. (c_parser_attributes): Move code into c_parser_attribute. (c_parser_unary_expression): Handle RID_HAS_ATTRIBUTE_EXPRESSION. gcc/c-family/ChangeLog: * c-attribs.c (type_for_vector_size): New function. (type_valid_for_vector_size): Same. (handle_vector_size_attribute): Move code to the functions above and call them. (validate_attribute, has_attribute): New functions. * c-common.h (has_attribute): Declare. (rid): Add RID_HAS_ATTRIBUTE_EXPRESSION. * c-common.c (c_common_resword): Same. gcc/cp/ChangeLog: * cp-tree.h (cp_check_const_attributes): Declare. * decl2.c (cp_check_const_attributes): Declare extern. * parser.c (cp_parser_has_attribute_expression): New function. (cp_parser_unary_expression): Handle RID_HAS_ATTRIBUTE_EXPRESSION. (cp_parser_gnu_attribute_list): Add argument. gcc/ChangeLog: * doc/extend.texi (Other Builtins): Add __builtin_has_attribute. gcc/testsuite/ChangeLog: * c-c++-common/builtin-has-attribute-2.c: New test. * c-c++-common/builtin-has-attribute-3.c: New test. * c-c++-common/builtin-has-attribute-4.c: New test. * c-c++-common/builtin-has-attribute.c: New test. * gcc.dg/builtin-has-attribute.c: New test. * gcc/testsuite/gcc.target/i386/builtin-has-attribute.c: New test. diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index 3a88766..c0a1b
Re: [PATCH] add simple attribute introspection
On 10/10/18, Martin Sebor wrote: > While writing tests for fixes and enhancements for attribute > handling I keep finding myself coming up with the same boiler- > plate code to verify whether an attribute has or has not been > successfully applied. It's often error-prone because it > depends on the subtle and unique effects each attribute has > on optimizers or code generation in general. > > Implementing an API just for testing might perhaps be excessive > but I suspect that users would find use for such a feature too. > I can even envision wanting to selectively apply attributes to > one's own symbols depending on their presence or absence on > other symbols (as an extension to the patch I posted for pr81824 > that introduces the copy attribute to copy attributes from one > symbol to another). > > The attached patch introduces a built-in function called > __builtin_has_attribute that makes some of this possible. > See the documentation and tests for details. __builtin_has_attribute sounds confusingly close to the __has_attribute macro, and yet from this description it sounds like it does something different. Maybe clear that up? > > The C++ implementation is only tested using the C tests and > I'm pretty sure it doesn't do the right thing for templates > but I think it can be extended to do that in a followup patch > or an update to this one. > > The C tests don't exhaustively exercise all attributes so it's > quite possible there are gaps/bugs where the built-in doesn't > return the right value due to missing special handling. > Attribute format validation is nearly non-existent. I view > these shortcomings as minor and they too can be plugged in > a followup patch. > > Ultimately, if this is viewed as useful as I'm hoping it will > be, I'd like to move the special handling from has_attribute > and to a callback function pointed from attribute_spec. That > way each attribute, front-end and back-end alike, could, in > addition the attribute handler, implement its own special > query routine without the details leaking into the rest of > the compiler. > > Martin >
Re: [PATCH] add simple attribute introspection
On Thu, 11 Oct 2018, Martin Sebor wrote: > (Or are there some differences between the underscored forms and > the one without it)? They should always behave the same. > With a few exceptions (like aligned) the built-in returns false > for attributes that aren't attached to a node. I haven't exercised > nearly all the attributes yet, and this one could very well be among > those that aren't and perhaps can't be handled. I suspect some > target attributes might be in the same group. If there's no way > to tell it should probably be documented as a limitation of > the function, maybe also under the attribute itself that can't > be detected. Alternatively, the built-in return type could be > changed to a tri-state: "don't know," false, true. Can you > think of a better solution? I don't have a better solution. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] add simple attribute introspection
On 10/11/2018 06:04 AM, Joseph Myers wrote: On Thu, 11 Oct 2018, Martin Sebor wrote: The attached patch introduces a built-in function called __builtin_has_attribute that makes some of this possible. See the documentation and tests for details. I see nothing in the documentation about handling of equivalent forms of an attribute - for example, specifying __aligned__ in the attribute but aligned in __builtin_has_attribute, or vice versa. I'd expect that to be documented to work (both of those should return true), with associated tests. (And likewise the semantics should allow for a format attribute using printf in one place and __printf__ in the other, for example, or the same constant argument represented with different expressions.) Yes, it occurred to me belatedly that I should add a test for those as well. I can also mention it in the documentation, although I'd have thought it would be implicit in how attributes work in general. (Or are there some differences between the underscored forms and the one without it)? What are the semantics of __builtin_has_attribute for attributes that can't be tested for? (E.g. the mode attribute, which ends up resulting in some existing type with the required mode being used, so there's nothing to indicate the attribute was originally used to declare things.) With a few exceptions (like aligned) the built-in returns false for attributes that aren't attached to a node. I haven't exercised nearly all the attributes yet, and this one could very well be among those that aren't and perhaps can't be handled. I suspect some target attributes might be in the same group. If there's no way to tell it should probably be documented as a limitation of the function, maybe also under the attribute itself that can't be detected. Alternatively, the built-in return type could be changed to a tri-state: "don't know," false, true. Can you think of a better solution? Martin
Re: [PATCH] add simple attribute introspection
On Thu, 11 Oct 2018, Martin Sebor wrote: > The attached patch introduces a built-in function called > __builtin_has_attribute that makes some of this possible. > See the documentation and tests for details. I see nothing in the documentation about handling of equivalent forms of an attribute - for example, specifying __aligned__ in the attribute but aligned in __builtin_has_attribute, or vice versa. I'd expect that to be documented to work (both of those should return true), with associated tests. (And likewise the semantics should allow for a format attribute using printf in one place and __printf__ in the other, for example, or the same constant argument represented with different expressions.) What are the semantics of __builtin_has_attribute for attributes that can't be tested for? (E.g. the mode attribute, which ends up resulting in some existing type with the required mode being used, so there's nothing to indicate the attribute was originally used to declare things.) -- Joseph S. Myers jos...@codesourcery.com
[PATCH] add simple attribute introspection
While writing tests for fixes and enhancements for attribute
handling I keep finding myself coming up with the same boiler-
plate code to verify whether an attribute has or has not been
successfully applied. It's often error-prone because it
depends on the subtle and unique effects each attribute has
on optimizers or code generation in general.
Implementing an API just for testing might perhaps be excessive
but I suspect that users would find use for such a feature too.
I can even envision wanting to selectively apply attributes to
one's own symbols depending on their presence or absence on
other symbols (as an extension to the patch I posted for pr81824
that introduces the copy attribute to copy attributes from one
symbol to another).
The attached patch introduces a built-in function called
__builtin_has_attribute that makes some of this possible.
See the documentation and tests for details.
The C++ implementation is only tested using the C tests and
I'm pretty sure it doesn't do the right thing for templates
but I think it can be extended to do that in a followup patch
or an update to this one.
The C tests don't exhaustively exercise all attributes so it's
quite possible there are gaps/bugs where the built-in doesn't
return the right value due to missing special handling.
Attribute format validation is nearly non-existent. I view
these shortcomings as minor and they too can be plugged in
a followup patch.
Ultimately, if this is viewed as useful as I'm hoping it will
be, I'd like to move the special handling from has_attribute
and to a callback function pointed from attribute_spec. That
way each attribute, front-end and back-end alike, could, in
addition the attribute handler, implement its own special
query routine without the details leaking into the rest of
the compiler.
Martin
gcc/c/ChangeLog:
* c-parser.c (c_parser_has_attribute_expression): New function.
(c_parser_attribute): New function.
(c_parser_attributes): Move code into c_parser_attribute.
(c_parser_unary_expression): Handle RID_HAS_ATTRIBUTE_EXPRESSION.
gcc/c-family/ChangeLog:
* c-attribs.c (validate_attribute, has_attribute): New functions.
* c-common.h (has_attribute): Declare.
* c-common.c (c_common_resword): Add RID_HAS_ATTRIBUTE_EXPRESSION.
* c-common.h (rid): Same.
gcc/cp/ChangeLog:
* cp-tree.h (cp_check_const_attributes): Declare.
* decl2.c (cp_check_const_attributes): Declare extern.
* parser.c (cp_parser_has_attribute_expression): New function.
(cp_parser_unary_expression): Handle RID_HAS_ATTRIBUTE_EXPRESSION.
(cp_parser_gnu_attribute_list): Add argument.
gcc/ChangeLog:
* doc/extend.texi (Other Builtins): Add __builtin_has_attribute.
gcc/testsuite/ChangeLog:
* c-c++-common/builtin-has-attribute-2.c: New test.
* c-c++-common/builtin-has-attribute-3.c: New test.
* c-c++-common/builtin-has-attribute.c: New test.
* gcc.dg/builtin-has-attribute.c: New test.
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 3a88766..56967fa 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -3678,3 +3678,220 @@ handle_patchable_function_entry_attribute (tree *, tree, tree, int, bool *)
/* Nothing to be done here. */
return NULL_TREE;
}
+
+/* Attempt to partially validate a single attribute ATTR if it were
+ to be applied to an entity OPER. */
+
+static bool
+validate_attribute (location_t atloc, tree oper, tree attr)
+{
+ /* Determine whether the name of the attribute is valid
+ and fail with an error if not. */
+ tree atname = get_attribute_name (attr);
+ if (!lookup_attribute_spec (atname))
+{
+ if (atloc != UNKNOWN_LOCATION)
+ error_at (atloc, "unknown attribute %qE", atname);
+ return false;
+}
+
+ if (!TREE_VALUE (attr))
+return true;
+
+ /* FIXME: Do some validation. */
+ if (!strcmp (IDENTIFIER_POINTER (atname), "format"))
+return true;
+
+ /* Only when attribute arguments have been provided try to validate
+ the whole thing. decl_attributes doesn't return an indication of
+ success or failure so proceed regardless. */
+ const char tmpname[] = "__builtin_has_attribute_tmp.";
+ tree tmpid = get_identifier (tmpname);
+ tree tmpdecl;
+ if (TYPE_P (oper))
+tmpdecl = build_decl (atloc, TYPE_DECL,
+ tmpid, oper);
+ else
+tmpdecl = build_decl (atloc, TREE_CODE (oper),
+ 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.) */
+ tree save_curfunc = current_function_decl;
+ current_function_decl = NULL_TREE;
+ if (DECL_P (tmpdecl))
+{
+ if (DECL_P (oper))
+ DECL_EXTERNAL (tmpdecl) = DECL_EXTERNAL (oper);
+ TREE_PUBLIC (tmpdecl) = TREE_PUBLIC (oper);
+}
+ decl_attributes (&tmpdecl, attr, 0);
+ current_function_decl = save_curfunc;
+
+ /* FIXME: Change decl_attributes to indicate success or failure. */
+ return true;
+}
+
+/* Return true if the DECL, EXPR, or
