Re: [PATCH][RFC] Canonize names of attributes.

[Jason Merrill]

On Wed, Jun 28, 2017 at 12:05 PM, Joseph Myers  wrote:
> On Wed, 28 Jun 2017, Martin Liška wrote:
>
>> On 06/14/2017 07:24 PM, Jason Merrill wrote:
>> > On Tue, Jun 13, 2017 at 8:32 AM, Martin Liška  wrote:
>> >> (canonize_attr_name): New function.
>> >
>> > I think this should be "canonicalize"; "canonize" means something else.
>> >
>> > Jason
>> >
>>
>> Yes, I hope it's a cosmetic problem. In general, do you welcome the change
>> to canonicalize attribute names?
>
> I think *names* (as opposed to arguments) should be canonicalized, but
> arguments need separate consideration.

I agree.

Jason

Re: [PATCH][RFC] Canonize names of attributes.

[Joseph Myers]

On Wed, 28 Jun 2017, Martin Liška wrote:

> On 06/14/2017 07:24 PM, Jason Merrill wrote:
> > On Tue, Jun 13, 2017 at 8:32 AM, Martin Liška  wrote:
> >> (canonize_attr_name): New function.
> > 
> > I think this should be "canonicalize"; "canonize" means something else.
> > 
> > Jason
> > 
> 
> Yes, I hope it's a cosmetic problem. In general, do you welcome the change
> to canonicalize attribute names?

I think *names* (as opposed to arguments) should be canonicalized, but 
arguments need separate consideration.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][RFC] Canonize names of attributes.

[Martin Liška]

On 06/14/2017 07:24 PM, Jason Merrill wrote:
> On Tue, Jun 13, 2017 at 8:32 AM, Martin Liška  wrote:
>> (canonize_attr_name): New function.
> 
> I think this should be "canonicalize"; "canonize" means something else.
> 
> Jason
> 

Yes, I hope it's a cosmetic problem. In general, do you welcome the change
to canonicalize attribute names?

Martin

Re: [PATCH][RFC] Canonize names of attributes.

[Martin Liška]

On 06/14/2017 06:40 PM, Joseph Myers wrote:
> On Wed, 14 Jun 2017, Richard Biener wrote:
> 
 are you sure this is needed?  This seems to be solely arguments to
 attributes.
>>>
>>> It's need for cases like:
>>>  __intN_t (8, __QI__);
>>
>> But __QI__ is not processed in lookup_attribute, is it?  So canonizing that
>> looks unrelated?  I didn't see similar handling in the C FE btw (but
>> maybe I missed it).
> 
> It's not clear to me that there is automatically a rule that where 
> identifiers are arguments to attributes, they must follow this rule about 
> foo and __foo__ being equivalent.
> 
> Specifically: c-attribs.c:attribute_takes_identifier_p says that the 
> cleanup attribute takes an identifier (a function name).  But it's 
> certainly the case that the exact function named there must be used; foo 
> and __foo__ as cleanup attribute arguments are not equivalent.  (You could 
> argue cleanup should take an expression, with an error then being given if 
> that doesn't evaluate to a function designator.)
> 

Hello.

This is obvious reason where original name must be preserved. Looks following 
patch
works and does not break test-suite:

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 8f638785e0e..08b4db5e5bd 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -24765,7 +24765,8 @@ cp_parser_gnu_attribute_list (cp_parser* parser)
  tree tv;
  if (arguments != NULL_TREE
  && ((tv = TREE_VALUE (arguments)) != NULL_TREE)
- && TREE_CODE (tv) == IDENTIFIER_NODE)
+ && TREE_CODE (tv) == IDENTIFIER_NODE
+ && !id_equal (TREE_PURPOSE (attribute), "cleanup"))
TREE_VALUE (arguments) = canonize_attr_name (tv);
  release_tree_vector (vec);
}
-- 

Well, the question is whether we want to in general canonicalize attribute 
names?
Special situations like "cleanup" can be handled.

Martin

Re: [PATCH][RFC] Canonize names of attributes.

[Martin Sebor]

On 06/14/2017 11:24 AM, Jason Merrill wrote:

On Tue, Jun 13, 2017 at 8:32 AM, Martin Liška  wrote:

(canonize_attr_name): New function.


I think this should be "canonicalize"; "canonize" means something else.


Right.  I was about to make the same observation but then grepped
GCC sources for the word and found a whole bunch of existing names
(besides canonize itself there's also canonize_uhwi and
df_canonize_collection_rec), so I bit my tongue.

Martin

Re: [PATCH][RFC] Canonize names of attributes.

[Jason Merrill]

On Tue, Jun 13, 2017 at 8:32 AM, Martin Liška  wrote:
> (canonize_attr_name): New function.

I think this should be "canonicalize"; "canonize" means something else.

Jason

Re: [PATCH][RFC] Canonize names of attributes.

[Joseph Myers]

On Wed, 14 Jun 2017, Richard Biener wrote:

> >> are you sure this is needed?  This seems to be solely arguments to
> >> attributes.
> >
> > It's need for cases like:
> >  __intN_t (8, __QI__);
> 
> But __QI__ is not processed in lookup_attribute, is it?  So canonizing that
> looks unrelated?  I didn't see similar handling in the C FE btw (but
> maybe I missed it).

It's not clear to me that there is automatically a rule that where 
identifiers are arguments to attributes, they must follow this rule about 
foo and __foo__ being equivalent.

Specifically: c-attribs.c:attribute_takes_identifier_p says that the 
cleanup attribute takes an identifier (a function name).  But it's 
certainly the case that the exact function named there must be used; foo 
and __foo__ as cleanup attribute arguments are not equivalent.  (You could 
argue cleanup should take an expression, with an error then being given if 
that doesn't evaluate to a function designator.)

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][RFC] Canonize names of attributes.

[Richard Biener]

On Wed, Jun 14, 2017 at 1:03 PM, Martin Liška  wrote:
> On 06/14/2017 11:07 AM, Richard Biener wrote:
>> On Wed, Jun 14, 2017 at 9:48 AM, Martin Liška  wrote:
>>> On 06/13/2017 03:20 PM, Richard Biener wrote:
 On Tue, Jun 13, 2017 at 2:32 PM, Martin Liška  wrote:
> Hello.
>
> After some discussions with Richi, I would like to propose patch that will
> come up with a canonical name of attribute names. That means 
> __attribute__((__abi_tag__))
> will be given 'abi_tag' as IDENTIFIER_NAME of the attribute. The change 
> can improve
> attribute name lookup and we can delete all the ugly code that compares 
> strlen(i1)
> == strlen(i2) + 4, etc.
>
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests 
> (w/ default
> languages). I'm currently testing objc, obj-c++ and go.
>
> Ready to be installed?


 +tree
 +canonize_attr_name (tree attr_name)
 +{

 needs a comment.
>>>
>>> Did that in header file.
>>
>> Coding convention requires it at the implementation.
>>

 +  if (l > 4 && s[0] == '_')
 +{
 +  gcc_assert (s[1] == '_');
 +  gcc_assert (s[l - 2] == '_');
 +  gcc_assert (s[l - 1] == '_');
 +  return get_identifier_with_length (s + 2, l - 4);
 +}

 a single gcc_checking_assert please.  I think this belongs in attribs.[ch].
>>>
>>> Ok, I'll put it there and make it static inline.
>>>

 Seeing

 diff --git a/gcc/c-family/c-lex.c b/gcc/c-family/c-lex.c
 index e1c8bdff986..6d0e9279ed6 100644
 --- a/gcc/c-family/c-lex.c
 +++ b/gcc/c-family/c-lex.c
 @@ -316,6 +316,7 @@ c_common_has_attribute (cpp_reader *pfile)
  {
attr_name = get_identifier ((const char *)
   cpp_token_as_text (pfile, token));
 +  attr_name = canonize_attr_name (attr_name);

 I wondered if we can save allocating the non-canonical identifier.  Like
 with

 tree
 canonize_attr_name (const char *attr_name, size_t len)

 as we can pass it IDENTIFIER_POINTER/LENGTH or the token.  OTOH
 all other cases do have IDENTIFIERs already...
>>>
>>> Well, back trace where identifiers are allocated is:
>>>
>>> #0  make_node_stat (code=code@entry=IDENTIFIER_NODE) at 
>>> ../../gcc/tree.c:1011
>>> #1  0x00da073e in alloc_node (table=) at 
>>> ../../gcc/stringpool.c:75
>>> #2  0x0163b49a in ht_lookup_with_hash (table=0x22f57b0,
>>> str=str@entry=0x2383615 "__abi_tag__ (\"cxx11\")))\n#endif\n\n\n#if 
>>> __cplusplus\n\n// Macro for constexpr, to support in mixed 03/0x 
>>> mode.\n#ifndef _GLIBCXX_CONSTEXPR\n# if __cplusplus >= 201103L\n#  define 
>>> _GLIBCXX_CONSTEXPR constexpr\n"..., len=len@entry=11,
>>> hash=hash@entry=144997377, insert=insert@entry=HT_ALLOC) at 
>>> ../../libcpp/symtab.c:155
>>> #3  0x0162d8ee in lex_identifier (pfile=pfile@entry=0x22f2fe0,
>>> base=0x2383615 "__abi_tag__ (\"cxx11\")))\n#endif\n\n\n#if 
>>> __cplusplus\n\n// Macro for constexpr, to support in mixed 03/0x 
>>> mode.\n#ifndef _GLIBCXX_CONSTEXPR\n# if __cplusplus >= 201103L\n#  define 
>>> _GLIBCXX_CONSTEXPR constexpr\n"...,
>>> starts_ucn=starts_ucn@entry=false, nst=nst@entry=0x7fffcd54, 
>>> spelling=spelling@entry=0x2348d98) at ../../libcpp/lex.c:1459
>>> #4  0x016304f0 in _cpp_lex_direct (pfile=pfile@entry=0x22f2fe0) at 
>>> ../../libcpp/lex.c:2788
>>>
>>> It's probably not possible to make a decision from this context about
>>> how an identifier will be used?
>>
>> No.
>>

 @ -24638,6 +24639,11 @@ cp_parser_gnu_attribute_list (cp_parser* parser)
   else
 {
   arguments = build_tree_list_vec (vec);
 + tree tv;
 + if (arguments != NULL_TREE
 + && ((tv = TREE_VALUE (arguments)) != NULL_TREE)
 + && TREE_CODE (tv) == IDENTIFIER_NODE)
 + TREE_VALUE (arguments) = canonize_attr_name (tv);
   release_tree_vector (vec);
 }

 are you sure this is needed?  This seems to be solely arguments to
 attributes.
>>>
>>> It's need for cases like:
>>>  __intN_t (8, __QI__);
>>
>> But __QI__ is not processed in lookup_attribute, is it?  So canonizing that
>> looks unrelated?  I didn't see similar handling in the C FE btw (but
>> maybe I missed it).
>
> It's for instance compared in cmp_attribs in:

Ah, of course.

Richard.

> /usr/include/stdio.h:391:62: internal compiler error: in cmp_attribs, at 
> c-family/c-format.c:3985
>   __THROWNL __attribute__ ((__format__ (__printf__, 3, 4)));
>   ^
> nf0xa86d86 cmp_attribs
> ../../gcc/c-family/c-format.c:3985
> 0xa86dfe convert_format_name_to_system_name
> ../../gcc/c-family/c-format.c:3967
> 

Re: [PATCH][RFC] Canonize names of attributes.

[Martin Liška]

On 06/14/2017 11:07 AM, Richard Biener wrote:
> On Wed, Jun 14, 2017 at 9:48 AM, Martin Liška  wrote:
>> On 06/13/2017 03:20 PM, Richard Biener wrote:
>>> On Tue, Jun 13, 2017 at 2:32 PM, Martin Liška  wrote:
 Hello.

 After some discussions with Richi, I would like to propose patch that will
 come up with a canonical name of attribute names. That means 
 __attribute__((__abi_tag__))
 will be given 'abi_tag' as IDENTIFIER_NAME of the attribute. The change 
 can improve
 attribute name lookup and we can delete all the ugly code that compares 
 strlen(i1)
 == strlen(i2) + 4, etc.

 Patch can bootstrap on ppc64le-redhat-linux and survives regression tests 
 (w/ default
 languages). I'm currently testing objc, obj-c++ and go.

 Ready to be installed?
>>>
>>>
>>> +tree
>>> +canonize_attr_name (tree attr_name)
>>> +{
>>>
>>> needs a comment.
>>
>> Did that in header file.
> 
> Coding convention requires it at the implementation.
> 
>>>
>>> +  if (l > 4 && s[0] == '_')
>>> +{
>>> +  gcc_assert (s[1] == '_');
>>> +  gcc_assert (s[l - 2] == '_');
>>> +  gcc_assert (s[l - 1] == '_');
>>> +  return get_identifier_with_length (s + 2, l - 4);
>>> +}
>>>
>>> a single gcc_checking_assert please.  I think this belongs in attribs.[ch].
>>
>> Ok, I'll put it there and make it static inline.
>>
>>>
>>> Seeing
>>>
>>> diff --git a/gcc/c-family/c-lex.c b/gcc/c-family/c-lex.c
>>> index e1c8bdff986..6d0e9279ed6 100644
>>> --- a/gcc/c-family/c-lex.c
>>> +++ b/gcc/c-family/c-lex.c
>>> @@ -316,6 +316,7 @@ c_common_has_attribute (cpp_reader *pfile)
>>>  {
>>>attr_name = get_identifier ((const char *)
>>>   cpp_token_as_text (pfile, token));
>>> +  attr_name = canonize_attr_name (attr_name);
>>>
>>> I wondered if we can save allocating the non-canonical identifier.  Like
>>> with
>>>
>>> tree
>>> canonize_attr_name (const char *attr_name, size_t len)
>>>
>>> as we can pass it IDENTIFIER_POINTER/LENGTH or the token.  OTOH
>>> all other cases do have IDENTIFIERs already...
>>
>> Well, back trace where identifiers are allocated is:
>>
>> #0  make_node_stat (code=code@entry=IDENTIFIER_NODE) at ../../gcc/tree.c:1011
>> #1  0x00da073e in alloc_node (table=) at 
>> ../../gcc/stringpool.c:75
>> #2  0x0163b49a in ht_lookup_with_hash (table=0x22f57b0,
>> str=str@entry=0x2383615 "__abi_tag__ (\"cxx11\")))\n#endif\n\n\n#if 
>> __cplusplus\n\n// Macro for constexpr, to support in mixed 03/0x 
>> mode.\n#ifndef _GLIBCXX_CONSTEXPR\n# if __cplusplus >= 201103L\n#  define 
>> _GLIBCXX_CONSTEXPR constexpr\n"..., len=len@entry=11,
>> hash=hash@entry=144997377, insert=insert@entry=HT_ALLOC) at 
>> ../../libcpp/symtab.c:155
>> #3  0x0162d8ee in lex_identifier (pfile=pfile@entry=0x22f2fe0,
>> base=0x2383615 "__abi_tag__ (\"cxx11\")))\n#endif\n\n\n#if 
>> __cplusplus\n\n// Macro for constexpr, to support in mixed 03/0x 
>> mode.\n#ifndef _GLIBCXX_CONSTEXPR\n# if __cplusplus >= 201103L\n#  define 
>> _GLIBCXX_CONSTEXPR constexpr\n"...,
>> starts_ucn=starts_ucn@entry=false, nst=nst@entry=0x7fffcd54, 
>> spelling=spelling@entry=0x2348d98) at ../../libcpp/lex.c:1459
>> #4  0x016304f0 in _cpp_lex_direct (pfile=pfile@entry=0x22f2fe0) at 
>> ../../libcpp/lex.c:2788
>>
>> It's probably not possible to make a decision from this context about
>> how an identifier will be used?
> 
> No.
> 
>>>
>>> @ -24638,6 +24639,11 @@ cp_parser_gnu_attribute_list (cp_parser* parser)
>>>   else
>>> {
>>>   arguments = build_tree_list_vec (vec);
>>> + tree tv;
>>> + if (arguments != NULL_TREE
>>> + && ((tv = TREE_VALUE (arguments)) != NULL_TREE)
>>> + && TREE_CODE (tv) == IDENTIFIER_NODE)
>>> + TREE_VALUE (arguments) = canonize_attr_name (tv);
>>>   release_tree_vector (vec);
>>> }
>>>
>>> are you sure this is needed?  This seems to be solely arguments to
>>> attributes.
>>
>> It's need for cases like:
>>  __intN_t (8, __QI__);
> 
> But __QI__ is not processed in lookup_attribute, is it?  So canonizing that
> looks unrelated?  I didn't see similar handling in the C FE btw (but
> maybe I missed it).

It's for instance compared in cmp_attribs in:

/usr/include/stdio.h:391:62: internal compiler error: in cmp_attribs, at 
c-family/c-format.c:3985
  __THROWNL __attribute__ ((__format__ (__printf__, 3, 4)));
  ^
nf0xa86d86 cmp_attribs
../../gcc/c-family/c-format.c:3985
0xa86dfe convert_format_name_to_system_name
../../gcc/c-family/c-format.c:3967
0xa8835c convert_format_name_to_system_name
../../gcc/c-family/c-format.c:339
0xa8835c decode_format_attr
../../gcc/c-family/c-format.c:300
0xa8b880 handle_format_attribute(tree_node**, tree_node*, tree

Re: [PATCH][RFC] Canonize names of attributes.

[Richard Biener]

On Wed, Jun 14, 2017 at 9:48 AM, Martin Liška  wrote:
> On 06/13/2017 03:20 PM, Richard Biener wrote:
>> On Tue, Jun 13, 2017 at 2:32 PM, Martin Liška  wrote:
>>> Hello.
>>>
>>> After some discussions with Richi, I would like to propose patch that will
>>> come up with a canonical name of attribute names. That means 
>>> __attribute__((__abi_tag__))
>>> will be given 'abi_tag' as IDENTIFIER_NAME of the attribute. The change can 
>>> improve
>>> attribute name lookup and we can delete all the ugly code that compares 
>>> strlen(i1)
>>> == strlen(i2) + 4, etc.
>>>
>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests 
>>> (w/ default
>>> languages). I'm currently testing objc, obj-c++ and go.
>>>
>>> Ready to be installed?
>>
>>
>> +tree
>> +canonize_attr_name (tree attr_name)
>> +{
>>
>> needs a comment.
>
> Did that in header file.

Coding convention requires it at the implementation.

>>
>> +  if (l > 4 && s[0] == '_')
>> +{
>> +  gcc_assert (s[1] == '_');
>> +  gcc_assert (s[l - 2] == '_');
>> +  gcc_assert (s[l - 1] == '_');
>> +  return get_identifier_with_length (s + 2, l - 4);
>> +}
>>
>> a single gcc_checking_assert please.  I think this belongs in attribs.[ch].
>
> Ok, I'll put it there and make it static inline.
>
>>
>> Seeing
>>
>> diff --git a/gcc/c-family/c-lex.c b/gcc/c-family/c-lex.c
>> index e1c8bdff986..6d0e9279ed6 100644
>> --- a/gcc/c-family/c-lex.c
>> +++ b/gcc/c-family/c-lex.c
>> @@ -316,6 +316,7 @@ c_common_has_attribute (cpp_reader *pfile)
>>  {
>>attr_name = get_identifier ((const char *)
>>   cpp_token_as_text (pfile, token));
>> +  attr_name = canonize_attr_name (attr_name);
>>
>> I wondered if we can save allocating the non-canonical identifier.  Like
>> with
>>
>> tree
>> canonize_attr_name (const char *attr_name, size_t len)
>>
>> as we can pass it IDENTIFIER_POINTER/LENGTH or the token.  OTOH
>> all other cases do have IDENTIFIERs already...
>
> Well, back trace where identifiers are allocated is:
>
> #0  make_node_stat (code=code@entry=IDENTIFIER_NODE) at ../../gcc/tree.c:1011
> #1  0x00da073e in alloc_node (table=) at 
> ../../gcc/stringpool.c:75
> #2  0x0163b49a in ht_lookup_with_hash (table=0x22f57b0,
> str=str@entry=0x2383615 "__abi_tag__ (\"cxx11\")))\n#endif\n\n\n#if 
> __cplusplus\n\n// Macro for constexpr, to support in mixed 03/0x 
> mode.\n#ifndef _GLIBCXX_CONSTEXPR\n# if __cplusplus >= 201103L\n#  define 
> _GLIBCXX_CONSTEXPR constexpr\n"..., len=len@entry=11,
> hash=hash@entry=144997377, insert=insert@entry=HT_ALLOC) at 
> ../../libcpp/symtab.c:155
> #3  0x0162d8ee in lex_identifier (pfile=pfile@entry=0x22f2fe0,
> base=0x2383615 "__abi_tag__ (\"cxx11\")))\n#endif\n\n\n#if 
> __cplusplus\n\n// Macro for constexpr, to support in mixed 03/0x 
> mode.\n#ifndef _GLIBCXX_CONSTEXPR\n# if __cplusplus >= 201103L\n#  define 
> _GLIBCXX_CONSTEXPR constexpr\n"...,
> starts_ucn=starts_ucn@entry=false, nst=nst@entry=0x7fffcd54, 
> spelling=spelling@entry=0x2348d98) at ../../libcpp/lex.c:1459
> #4  0x016304f0 in _cpp_lex_direct (pfile=pfile@entry=0x22f2fe0) at 
> ../../libcpp/lex.c:2788
>
> It's probably not possible to make a decision from this context about
> how an identifier will be used?

No.

>>
>> @ -24638,6 +24639,11 @@ cp_parser_gnu_attribute_list (cp_parser* parser)
>>   else
>> {
>>   arguments = build_tree_list_vec (vec);
>> + tree tv;
>> + if (arguments != NULL_TREE
>> + && ((tv = TREE_VALUE (arguments)) != NULL_TREE)
>> + && TREE_CODE (tv) == IDENTIFIER_NODE)
>> + TREE_VALUE (arguments) = canonize_attr_name (tv);
>>   release_tree_vector (vec);
>> }
>>
>> are you sure this is needed?  This seems to be solely arguments to
>> attributes.
>
> It's need for cases like:
>  __intN_t (8, __QI__);

But __QI__ is not processed in lookup_attribute, is it?  So canonizing that
looks unrelated?  I didn't see similar handling in the C FE btw (but
maybe I missed it).

>>
>> The rest of the changes look good but please wait for input from FE 
>> maintainers.
>
> Good, I'm attaching v2 and CCing FE maintainers.
>
> Martin
>
>>
>> Thanks,
>> Richard.
>>
>>> Martin
>>>
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 2017-06-09  Martin Liska  
>>>
>>> * parser.c (cp_parser_gnu_attribute_list): Canonize attribute
>>> names.
>>> (cp_parser_std_attribute): Likewise.
>>>
>>> gcc/go/ChangeLog:
>>>
>>> 2017-06-09  Martin Liska  
>>>
>>> * go-gcc.cc (Gcc_backend::function): Use no_split_stack
>>> instead of __no_split_stack__.
>>>
>>> gcc/c/ChangeLog:
>>>
>>> 2017-06-09  Martin Liska  
>>>
>>> * c-parser.c (c_parser_attributes): Canonize attribute names.
>>>
>>> gcc/c-family/ChangeLog:
>>>
>>> 2017-06-09  Martin Liska  
>>>
>>> * c-format.c 

Re: [PATCH][RFC] Canonize names of attributes.

[Martin Liška]

On 06/13/2017 03:20 PM, Richard Biener wrote:
> On Tue, Jun 13, 2017 at 2:32 PM, Martin Liška  wrote:
>> Hello.
>>
>> After some discussions with Richi, I would like to propose patch that will
>> come up with a canonical name of attribute names. That means 
>> __attribute__((__abi_tag__))
>> will be given 'abi_tag' as IDENTIFIER_NAME of the attribute. The change can 
>> improve
>> attribute name lookup and we can delete all the ugly code that compares 
>> strlen(i1)
>> == strlen(i2) + 4, etc.
>>
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests 
>> (w/ default
>> languages). I'm currently testing objc, obj-c++ and go.
>>
>> Ready to be installed?
> 
> 
> +tree
> +canonize_attr_name (tree attr_name)
> +{
> 
> needs a comment.

Did that in header file.

> 
> +  if (l > 4 && s[0] == '_')
> +{
> +  gcc_assert (s[1] == '_');
> +  gcc_assert (s[l - 2] == '_');
> +  gcc_assert (s[l - 1] == '_');
> +  return get_identifier_with_length (s + 2, l - 4);
> +}
> 
> a single gcc_checking_assert please.  I think this belongs in attribs.[ch].

Ok, I'll put it there and make it static inline.

> 
> Seeing
> 
> diff --git a/gcc/c-family/c-lex.c b/gcc/c-family/c-lex.c
> index e1c8bdff986..6d0e9279ed6 100644
> --- a/gcc/c-family/c-lex.c
> +++ b/gcc/c-family/c-lex.c
> @@ -316,6 +316,7 @@ c_common_has_attribute (cpp_reader *pfile)
>  {
>attr_name = get_identifier ((const char *)
>   cpp_token_as_text (pfile, token));
> +  attr_name = canonize_attr_name (attr_name);
> 
> I wondered if we can save allocating the non-canonical identifier.  Like
> with
> 
> tree
> canonize_attr_name (const char *attr_name, size_t len)
> 
> as we can pass it IDENTIFIER_POINTER/LENGTH or the token.  OTOH
> all other cases do have IDENTIFIERs already...

Well, back trace where identifiers are allocated is:

#0  make_node_stat (code=code@entry=IDENTIFIER_NODE) at ../../gcc/tree.c:1011
#1  0x00da073e in alloc_node (table=) at 
../../gcc/stringpool.c:75
#2  0x0163b49a in ht_lookup_with_hash (table=0x22f57b0, 
str=str@entry=0x2383615 "__abi_tag__ (\"cxx11\")))\n#endif\n\n\n#if 
__cplusplus\n\n// Macro for constexpr, to support in mixed 03/0x mode.\n#ifndef 
_GLIBCXX_CONSTEXPR\n# if __cplusplus >= 201103L\n#  define _GLIBCXX_CONSTEXPR 
constexpr\n"..., len=len@entry=11, 
hash=hash@entry=144997377, insert=insert@entry=HT_ALLOC) at 
../../libcpp/symtab.c:155
#3  0x0162d8ee in lex_identifier (pfile=pfile@entry=0x22f2fe0, 
base=0x2383615 "__abi_tag__ (\"cxx11\")))\n#endif\n\n\n#if 
__cplusplus\n\n// Macro for constexpr, to support in mixed 03/0x mode.\n#ifndef 
_GLIBCXX_CONSTEXPR\n# if __cplusplus >= 201103L\n#  define _GLIBCXX_CONSTEXPR 
constexpr\n"..., 
starts_ucn=starts_ucn@entry=false, nst=nst@entry=0x7fffcd54, 
spelling=spelling@entry=0x2348d98) at ../../libcpp/lex.c:1459
#4  0x016304f0 in _cpp_lex_direct (pfile=pfile@entry=0x22f2fe0) at 
../../libcpp/lex.c:2788

It's probably not possible to make a decision from this context about
how an identifier will be used?

> 
> @ -24638,6 +24639,11 @@ cp_parser_gnu_attribute_list (cp_parser* parser)
>   else
> {
>   arguments = build_tree_list_vec (vec);
> + tree tv;
> + if (arguments != NULL_TREE
> + && ((tv = TREE_VALUE (arguments)) != NULL_TREE)
> + && TREE_CODE (tv) == IDENTIFIER_NODE)
> + TREE_VALUE (arguments) = canonize_attr_name (tv);
>   release_tree_vector (vec);
> }
> 
> are you sure this is needed?  This seems to be solely arguments to
> attributes.

It's need for cases like:
 __intN_t (8, __QI__);

> 
> The rest of the changes look good but please wait for input from FE 
> maintainers.

Good, I'm attaching v2 and CCing FE maintainers.

Martin

> 
> Thanks,
> Richard.
> 
>> Martin
>>
>>
>> gcc/cp/ChangeLog:
>>
>> 2017-06-09  Martin Liska  
>>
>> * parser.c (cp_parser_gnu_attribute_list): Canonize attribute
>> names.
>> (cp_parser_std_attribute): Likewise.
>>
>> gcc/go/ChangeLog:
>>
>> 2017-06-09  Martin Liska  
>>
>> * go-gcc.cc (Gcc_backend::function): Use no_split_stack
>> instead of __no_split_stack__.
>>
>> gcc/c/ChangeLog:
>>
>> 2017-06-09  Martin Liska  
>>
>> * c-parser.c (c_parser_attributes): Canonize attribute names.
>>
>> gcc/c-family/ChangeLog:
>>
>> 2017-06-09  Martin Liska  
>>
>> * c-format.c (cmp_attribs): Simplify comparison of attributes.
>> * c-lex.c (c_common_has_attribute): Canonize attribute names.
>>
>> gcc/ChangeLog:
>>
>> 2017-06-09  Martin Liska  
>>
>> * tree.c (cmp_attrib_identifiers): Simplify comparison of attributes.
>> (private_is_attribute_p): Likewise.
>> (private_lookup_attribute): Likewise.
>> (private_lookup_attribute_by_prefix): Likewise.
>> (remove

Re: [PATCH][RFC] Canonize names of attributes.

[Richard Biener]

On Tue, Jun 13, 2017 at 2:32 PM, Martin Liška  wrote:
> Hello.
>
> After some discussions with Richi, I would like to propose patch that will
> come up with a canonical name of attribute names. That means 
> __attribute__((__abi_tag__))
> will be given 'abi_tag' as IDENTIFIER_NAME of the attribute. The change can 
> improve
> attribute name lookup and we can delete all the ugly code that compares 
> strlen(i1)
> == strlen(i2) + 4, etc.
>
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests (w/ 
> default
> languages). I'm currently testing objc, obj-c++ and go.
>
> Ready to be installed?


+tree
+canonize_attr_name (tree attr_name)
+{

needs a comment.

+  if (l > 4 && s[0] == '_')
+{
+  gcc_assert (s[1] == '_');
+  gcc_assert (s[l - 2] == '_');
+  gcc_assert (s[l - 1] == '_');
+  return get_identifier_with_length (s + 2, l - 4);
+}

a single gcc_checking_assert please.  I think this belongs in attribs.[ch].

Seeing

diff --git a/gcc/c-family/c-lex.c b/gcc/c-family/c-lex.c
index e1c8bdff986..6d0e9279ed6 100644
--- a/gcc/c-family/c-lex.c
+++ b/gcc/c-family/c-lex.c
@@ -316,6 +316,7 @@ c_common_has_attribute (cpp_reader *pfile)
 {
   attr_name = get_identifier ((const char *)
  cpp_token_as_text (pfile, token));
+  attr_name = canonize_attr_name (attr_name);

I wondered if we can save allocating the non-canonical identifier.  Like
with

tree
canonize_attr_name (const char *attr_name, size_t len)

as we can pass it IDENTIFIER_POINTER/LENGTH or the token.  OTOH
all other cases do have IDENTIFIERs already...

@ -24638,6 +24639,11 @@ cp_parser_gnu_attribute_list (cp_parser* parser)
  else
{
  arguments = build_tree_list_vec (vec);
+ tree tv;
+ if (arguments != NULL_TREE
+ && ((tv = TREE_VALUE (arguments)) != NULL_TREE)
+ && TREE_CODE (tv) == IDENTIFIER_NODE)
+ TREE_VALUE (arguments) = canonize_attr_name (tv);
  release_tree_vector (vec);
}

are you sure this is needed?  This seems to be solely arguments to
attributes.

The rest of the changes look good but please wait for input from FE maintainers.

Thanks,
Richard.

> Martin
>
>
> gcc/cp/ChangeLog:
>
> 2017-06-09  Martin Liska  
>
> * parser.c (cp_parser_gnu_attribute_list): Canonize attribute
> names.
> (cp_parser_std_attribute): Likewise.
>
> gcc/go/ChangeLog:
>
> 2017-06-09  Martin Liska  
>
> * go-gcc.cc (Gcc_backend::function): Use no_split_stack
> instead of __no_split_stack__.
>
> gcc/c/ChangeLog:
>
> 2017-06-09  Martin Liska  
>
> * c-parser.c (c_parser_attributes): Canonize attribute names.
>
> gcc/c-family/ChangeLog:
>
> 2017-06-09  Martin Liska  
>
> * c-format.c (cmp_attribs): Simplify comparison of attributes.
> * c-lex.c (c_common_has_attribute): Canonize attribute names.
>
> gcc/ChangeLog:
>
> 2017-06-09  Martin Liska  
>
> * tree.c (cmp_attrib_identifiers): Simplify comparison of attributes.
> (private_is_attribute_p): Likewise.
> (private_lookup_attribute): Likewise.
> (private_lookup_attribute_by_prefix): Likewise.
> (remove_attribute): Likewise.
> (canonize_attr_name): New function.
> * tree.h: Declared here.
>
> gcc/testsuite/ChangeLog:
>
> 2017-06-09  Martin Liska  
>
> * g++.dg/cpp0x/pr65558.C: Change expected warning.
> * gcc.dg/parm-impl-decl-1.c: Likewise.
> * gcc.dg/parm-impl-decl-3.c: Likewise.
> ---
>  gcc/c-family/c-format.c |  13 ++--
>  gcc/c-family/c-lex.c|   1 +
>  gcc/c/c-parser.c|   9 +++
>  gcc/cp/parser.c |  11 +++-
>  gcc/go/go-gcc.cc|   2 +-
>  gcc/testsuite/g++.dg/cpp0x/pr65558.C|   2 +-
>  gcc/testsuite/gcc.dg/parm-impl-decl-1.c |   2 +-
>  gcc/testsuite/gcc.dg/parm-impl-decl-3.c |   2 +-
>  gcc/tree.c  | 108 
> +++-
>  gcc/tree.h  |   4 ++
>  10 files changed, 69 insertions(+), 85 deletions(-)
>
>