On Wed, Jun 14, 2017 at 9:48 AM, Martin Liška <mli...@suse.cz> wrote:
> On 06/13/2017 03:20 PM, Richard Biener wrote:
>> On Tue, Jun 13, 2017 at 2:32 PM, Martin Liška <mli...@suse.cz> 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 0x0000000000da073e in alloc_node (table=<optimized out>) at
> ../../gcc/stringpool.c:75
> #2 0x000000000163b49a 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 0x000000000162d8ee 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=0x7fffffffcd54,
> spelling=spelling@entry=0x2348d98) at ../../libcpp/lex.c:1459
> #4 0x00000000016304f0 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 <mli...@suse.cz>
>>>
>>> * parser.c (cp_parser_gnu_attribute_list): Canonize attribute
>>> names.
>>> (cp_parser_std_attribute): Likewise.
>>>
>>> gcc/go/ChangeLog:
>>>
>>> 2017-06-09 Martin Liska <mli...@suse.cz>
>>>
>>> * go-gcc.cc (Gcc_backend::function): Use no_split_stack
>>> instead of __no_split_stack__.
>>>
>>> gcc/c/ChangeLog:
>>>
>>> 2017-06-09 Martin Liska <mli...@suse.cz>
>>>
>>> * c-parser.c (c_parser_attributes): Canonize attribute names.
>>>
>>> gcc/c-family/ChangeLog:
>>>
>>> 2017-06-09 Martin Liska <mli...@suse.cz>
>>>
>>> * 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 <mli...@suse.cz>
>>>
>>> * 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 <mli...@suse.cz>
>>>
>>> * 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(-)
>>>
>>>
>
