On 06/14/2017 11:07 AM, Richard Biener wrote:
> 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).
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_node*, int,
bool*)
../../gcc/c-family/c-format.c:4019
0xa49fb7 decl_attributes(tree_node**, tree_node*, int)
../../gcc/attribs.c:548
0x866dd3 cplus_decl_attributes(tree_node**, tree_node*, int)
../../gcc/cp/decl2.c:1431
0x7786a1 grokfndecl
../../gcc/cp/decl.c:8836
0x85127d grokdeclarator(cp_declarator const*, cp_decl_specifier_seq*,
decl_context, int, tree_node**)
../../gcc/cp/decl.c:12215
0x8542a6 start_decl(cp_declarator const*, cp_decl_specifier_seq*, int,
tree_node*, tree_node*, tree_node**)
../../gcc/cp/decl.c:4922
0x910de7 cp_parser_init_declarator
../../gcc/cp/parser.c:19268
0x905145 cp_parser_simple_declaration
../../gcc/cp/parser.c:12804
0x904cae cp_parser_block_declaration
../../gcc/cp/parser.c:12629
0x904a30 cp_parser_declaration
../../gcc/cp/parser.c:12527
0x904589 cp_parser_declaration_seq_opt
../../gcc/cp/parser.c:12403
0x9066a5 cp_parser_linkage_specification
../../gcc/cp/parser.c:13557
0x9047ad cp_parser_declaration
../../gcc/cp/parser.c:12464
0x904589 cp_parser_declaration_seq_opt
../../gcc/cp/parser.c:12403
0x8f31b7 cp_parser_translation_unit
../../gcc/cp/parser.c:4364
0x94402a c_parse_file()
../../gcc/cp/parser.c:38486
Martin
>
>>>
>>> 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(-)
>>>>
>>>>
>>
