This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 06/14/2017 11:07 AM, Richard Biener wrote:
> On Wed, Jun 14, 2017 at 9:48 AM, Martin Liška <mliska@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 <mliska@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  <mliska@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  <mliska@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  <mliska@suse.cz>
>>>>
>>>>         * c-parser.c (c_parser_attributes): Canonize attribute names.
>>>>
>>>> gcc/c-family/ChangeLog:
>>>>
>>>> 2017-06-09  Martin Liska  <mliska@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  <mliska@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  <mliska@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(-)
>>>>
>>>>
>>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]