On Wed, Jun 14, 2017 at 1:03 PM, Martin Liška <mli...@suse.cz> wrote:
> 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:
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
> 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(-)
>>>>>
>>>>>
>>>
>
