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. > > + 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? > > @ -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 <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(-) >> >>
>From ab39c1ba7011ceaea8c71f65b7188c0249b3c409 Mon Sep 17 00:00:00 2001 From: marxin <mli...@suse.cz> Date: Thu, 8 Jun 2017 10:23:25 +0200 Subject: [PATCH] Canonize names of attributes. 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. * tree.c: Include stringpool.h header file. 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. * c-pretty-print.c: Include stringpool.h header file. gcc/ChangeLog: 2017-06-09 Martin Liska <mli...@suse.cz> * attribs.h (canonize_attr_name): New function. * 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. 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/attribs.h | 19 +++++++ gcc/c-family/c-format.c | 13 ++--- gcc/c-family/c-lex.c | 1 + gcc/c-family/c-pretty-print.c | 1 + gcc/c/c-parser.c | 9 ++++ gcc/cp/parser.c | 11 +++- gcc/cp/tree.c | 1 + 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 | 91 ++++++++------------------------- 12 files changed, 69 insertions(+), 85 deletions(-) diff --git a/gcc/attribs.h b/gcc/attribs.h index 7f13332700e..010a4e5165d 100644 --- a/gcc/attribs.h +++ b/gcc/attribs.h @@ -47,4 +47,23 @@ extern char *make_unique_name (tree, const char *, bool); extern tree make_dispatcher_decl (const tree); extern bool is_function_default_version (const tree); +/* For a given IDENTIFIER_NODE, strip leading and trailing '_' characters + so that we have a canonized shape or attribute names. */ + +static inline tree +canonize_attr_name (tree attr_name) +{ + const size_t l = IDENTIFIER_LENGTH (attr_name); + const char *s = IDENTIFIER_POINTER (attr_name); + + if (l > 4 && s[0] == '_') + { + gcc_checking_assert (s[l - 2] == '_'); + return get_identifier_with_length (s + 2, l - 4); + } + + return attr_name; +} + + #endif // GCC_ATTRIBS_H diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c index 732339b9b5e..30f60d42cca 100644 --- a/gcc/c-family/c-format.c +++ b/gcc/c-family/c-format.c @@ -3982,15 +3982,10 @@ cmp_attribs (const char *tattr_name, const char *attr_name) { int alen = strlen (attr_name); int slen = (tattr_name ? strlen (tattr_name) : 0); - if (alen > 4 && attr_name[0] == '_' && attr_name[1] == '_' - && attr_name[alen - 1] == '_' && attr_name[alen - 2] == '_') - { - attr_name += 2; - alen -= 4; - } - if (alen != slen || strncmp (tattr_name, attr_name, alen) != 0) - return false; - return true; + gcc_checking_assert (alen == 0 || attr_name[0] != '_'); + gcc_checking_assert (slen == 0 || tattr_name[0] != '_'); + + return (alen == slen && strncmp (tattr_name, attr_name, alen) == 0); } /* Handle a "format" attribute; arguments as in 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); if (c_dialect_cxx ()) { int idx = 0; diff --git a/gcc/c-family/c-pretty-print.c b/gcc/c-family/c-pretty-print.c index fdb7b41f592..b8b8f665ef3 100644 --- a/gcc/c-family/c-pretty-print.c +++ b/gcc/c-family/c-pretty-print.c @@ -24,6 +24,7 @@ along with GCC; see the file COPYING3. If not see #include "c-pretty-print.h" #include "diagnostic.h" #include "stor-layout.h" +#include "stringpool.h" #include "attribs.h" #include "intl.h" #include "tree-pretty-print.h" diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index 6f954f21fa2..400b65380e2 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -4168,9 +4168,11 @@ c_parser_attributes (c_parser *parser) attr_name = c_parser_attribute_any_word (parser); if (attr_name == NULL) break; + attr_name = canonize_attr_name (attr_name); if (is_cilkplus_vector_p (attr_name)) { c_token *v_token = c_parser_peek_token (parser); + v_token->value = canonize_attr_name (v_token->value); c_parser_cilk_simd_fn_vector_attrs (parser, *v_token); /* If the next token isn't a comma, we're done. */ if (!c_parser_next_token_is (parser, CPP_COMMA)) @@ -4234,6 +4236,13 @@ c_parser_attributes (c_parser *parser) release_tree_vector (expr_list); } } + + if (attr_args + && TREE_VALUE (attr_args) + && TREE_CODE (TREE_VALUE (attr_args)) == IDENTIFIER_NODE) + TREE_VALUE (attr_args) + = canonize_attr_name (TREE_VALUE (attr_args)); + attr = build_tree_list (attr_name, attr_args); if (c_parser_next_token_is (parser, CPP_CLOSE_PAREN)) c_parser_consume_token (parser); diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index d02ad360d16..b968ed6572e 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -24612,7 +24612,8 @@ cp_parser_gnu_attribute_list (cp_parser* parser) parsed identifier. */ ? ridpointers[(int) token->keyword] : id_token->u.value; - + + identifier = canonize_attr_name (identifier); attribute = build_tree_list (identifier, NULL_TREE); /* Peek at the next token. */ @@ -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); } /* Save the arguments away. */ @@ -24758,6 +24764,8 @@ cp_parser_std_attribute (cp_parser *parser, tree attr_ns) "expected an identifier for the attribute name"); return error_mark_node; } + + attr_id = canonize_attr_name (attr_id); attribute = build_tree_list (build_tree_list (attr_ns, attr_id), NULL_TREE); token = cp_lexer_peek_token (parser->lexer); @@ -24767,6 +24775,7 @@ cp_parser_std_attribute (cp_parser *parser, tree attr_ns) NULL_TREE); else { + attr_id = canonize_attr_name (attr_id); attribute = build_tree_list (build_tree_list (NULL_TREE, attr_id), NULL_TREE); /* C++11 noreturn attribute is equivalent to GNU's. */ diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c index bb17278c611..513ecf6e71b 100644 --- a/gcc/cp/tree.c +++ b/gcc/cp/tree.c @@ -32,6 +32,7 @@ along with GCC; see the file COPYING3. If not see #include "debug.h" #include "convert.h" #include "gimplify.h" +#include "stringpool.h" #include "attribs.h" #include "flags.h" diff --git a/gcc/go/go-gcc.cc b/gcc/go/go-gcc.cc index a7977fe03c1..04912f0ed01 100644 --- a/gcc/go/go-gcc.cc +++ b/gcc/go/go-gcc.cc @@ -3046,7 +3046,7 @@ Gcc_backend::function(Btype* fntype, const std::string& name, DECL_UNINLINABLE(decl) = 1; if (disable_split_stack) { - tree attr = get_identifier("__no_split_stack__"); + tree attr = get_identifier ("no_split_stack"); DECL_ATTRIBUTES(decl) = tree_cons(attr, NULL_TREE, NULL_TREE); } if (in_unique_section) diff --git a/gcc/testsuite/g++.dg/cpp0x/pr65558.C b/gcc/testsuite/g++.dg/cpp0x/pr65558.C index d294c95a657..12946b35eda 100644 --- a/gcc/testsuite/g++.dg/cpp0x/pr65558.C +++ b/gcc/testsuite/g++.dg/cpp0x/pr65558.C @@ -2,6 +2,6 @@ // { dg-do compile { target c++11 } } inline namespace -__attribute__((__abi_tag__)) // { dg-warning "ignoring .__abi_tag__. attribute on anonymous namespace" } +__attribute__((__abi_tag__)) // { dg-warning "ignoring .abi_tag. attribute on anonymous namespace" } { } diff --git a/gcc/testsuite/gcc.dg/parm-impl-decl-1.c b/gcc/testsuite/gcc.dg/parm-impl-decl-1.c index 5c7ddb0a259..c1219273c75 100644 --- a/gcc/testsuite/gcc.dg/parm-impl-decl-1.c +++ b/gcc/testsuite/gcc.dg/parm-impl-decl-1.c @@ -7,7 +7,7 @@ /* Implicit function declaration in attribute in definition (testcase from bug). */ int -foo (int __attribute__ ((__mode__ (vector_size(8)))) i) /* { dg-warning "'__mode__' attribute ignored" } */ +foo (int __attribute__ ((__mode__ (vector_size(8)))) i) /* { dg-warning "'mode' attribute ignored" } */ { return (long long) i; } diff --git a/gcc/testsuite/gcc.dg/parm-impl-decl-3.c b/gcc/testsuite/gcc.dg/parm-impl-decl-3.c index 904295258d7..20197b52402 100644 --- a/gcc/testsuite/gcc.dg/parm-impl-decl-3.c +++ b/gcc/testsuite/gcc.dg/parm-impl-decl-3.c @@ -4,7 +4,7 @@ /* { dg-options "-g -std=gnu89" } */ int -foo (int __attribute__ ((__mode__ (vector_size(8)))) i) /* { dg-warning "'__mode__' attribute ignored" } */ +foo (int __attribute__ ((__mode__ (vector_size(8)))) i) /* { dg-warning "'mode' attribute ignored" } */ { return (long long) i; } diff --git a/gcc/tree.c b/gcc/tree.c index 260280317bc..cfacae8a506 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -4943,34 +4943,16 @@ cmp_attrib_identifiers (const_tree attr1, const_tree attr2) if (attr1 == attr2) return true; - /* If they are not equal, they may still be one in the form - 'text' while the other one is in the form '__text__'. TODO: - If we were storing attributes in normalized 'text' form, then - this could all go away and we could take full advantage of - the fact that we're comparing identifiers. :-) */ const size_t attr1_len = IDENTIFIER_LENGTH (attr1); const size_t attr2_len = IDENTIFIER_LENGTH (attr2); - if (attr2_len == attr1_len + 4) - { - const char *p = IDENTIFIER_POINTER (attr2); - const char *q = IDENTIFIER_POINTER (attr1); - if (p[0] == '_' && p[1] == '_' - && p[attr2_len - 2] == '_' && p[attr2_len - 1] == '_' - && strncmp (q, p + 2, attr1_len) == 0) - return true;; - } - else if (attr2_len + 4 == attr1_len) - { - const char *p = IDENTIFIER_POINTER (attr2); - const char *q = IDENTIFIER_POINTER (attr1); - if (q[0] == '_' && q[1] == '_' - && q[attr1_len - 2] == '_' && q[attr1_len - 1] == '_' - && strncmp (q + 2, p, attr2_len) == 0) - return true; - } + if (attr1_len != attr2_len) + return false; + gcc_checking_assert (IDENTIFIER_POINTER (attr1)[0] != '_'); + gcc_checking_assert (IDENTIFIER_POINTER (attr2)[0] != '_'); - return false; + return strncmp (IDENTIFIER_POINTER (attr1), IDENTIFIER_POINTER (attr2), + attr1_len) == 0; } /* Compare two attributes for their value identity. Return true if the @@ -6047,24 +6029,9 @@ bool private_is_attribute_p (const char *attr_name, size_t attr_len, const_tree ident) { size_t ident_len = IDENTIFIER_LENGTH (ident); + gcc_checking_assert (ident_len == 0 || IDENTIFIER_POINTER (ident)[0] != '_'); - if (ident_len == attr_len) - { - if (id_equal (ident, attr_name)) - return true; - } - else if (ident_len == attr_len + 4) - { - /* There is the possibility that ATTR is 'text' and IDENT is - '__text__'. */ - const char *p = IDENTIFIER_POINTER (ident); - if (p[0] == '_' && p[1] == '_' - && p[ident_len - 2] == '_' && p[ident_len - 1] == '_' - && strncmp (attr_name, p + 2, attr_len) == 0) - return true; - } - - return false; + return ident_len == attr_len && id_equal (ident, attr_name); } /* The backbone of lookup_attribute(). ATTR_LEN is the string length @@ -6074,25 +6041,13 @@ private_lookup_attribute (const char *attr_name, size_t attr_len, tree list) { while (list) { - size_t ident_len = IDENTIFIER_LENGTH (get_attribute_name (list)); - - if (ident_len == attr_len) - { - if (!strcmp (attr_name, - IDENTIFIER_POINTER (get_attribute_name (list)))) + tree attr = get_attribute_name (list); + size_t ident_len = IDENTIFIER_LENGTH (attr); + gcc_checking_assert (ident_len == 0 + || IDENTIFIER_POINTER (attr)[0] != '_'); + if (ident_len == attr_len + && strcmp (attr_name, IDENTIFIER_POINTER (attr)) == 0) break; - } - /* TODO: If we made sure that attributes were stored in the - canonical form without '__...__' (ie, as in 'text' as opposed - to '__text__') then we could avoid the following case. */ - else if (ident_len == attr_len + 4) - { - const char *p = IDENTIFIER_POINTER (get_attribute_name (list)); - if (p[0] == '_' && p[1] == '_' - && p[ident_len - 2] == '_' && p[ident_len - 1] == '_' - && strncmp (attr_name, p + 2, attr_len) == 0) - break; - } list = TREE_CHAIN (list); } @@ -6119,17 +6074,11 @@ private_lookup_attribute_by_prefix (const char *attr_name, size_t attr_len, } const char *p = IDENTIFIER_POINTER (get_attribute_name (list)); + gcc_checking_assert (attr_len == 0 || p[0] != '_'); if (strncmp (attr_name, p, attr_len) == 0) break; - /* TODO: If we made sure that attributes were stored in the - canonical form without '__...__' (ie, as in 'text' as opposed - to '__text__') then we could avoid the following case. */ - if (p[0] == '_' && p[1] == '_' && - strncmp (attr_name, p + 2, attr_len) == 0) - break; - list = TREE_CHAIN (list); } @@ -6175,16 +6124,16 @@ tree remove_attribute (const char *attr_name, tree list) { tree *p; - size_t attr_len = strlen (attr_name); - gcc_checking_assert (attr_name[0] != '_'); for (p = &list; *p; ) { tree l = *p; - /* TODO: If we were storing attributes in normalized form, here - we could use a simple strcmp(). */ - if (private_is_attribute_p (attr_name, attr_len, get_attribute_name (l))) + + tree attr = get_attribute_name (l); + gcc_checking_assert (IDENTIFIER_LENGTH (attr) == 0 + || IDENTIFIER_POINTER (attr)[0] != '_'); + if (strcmp (attr_name, IDENTIFIER_POINTER (attr)) == 0) *p = TREE_CHAIN (l); else p = &TREE_CHAIN (l); -- 2.13.1