On Fri, Jul 14, 2023 at 5:58 PM Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Summary: We'd like to be able to specify some attributes using > keywords, rather than the traditional __attribute__ or [[...]] > syntax. Would that be OK? > > In more detail: > > We'd like to add some new target-specific attributes for Arm SME. > These attributes affect semantics and code generation and so they > can't simply be ignored. > > Traditionally we've done this kind of thing by adding GNU attributes, > via TARGET_ATTRIBUTE_TABLE in GCC's case. The problem is that both > GCC and Clang have traditionally only warned about unrecognised GNU > attributes, rather than raising an error. Older compilers might > therefore be able to look past some uses of the new attributes and > still produce object code, even though that object code is almost > certainly going to be wrong. (The compilers will also emit a default-on > warning, but that might go unnoticed when building a big project.) > > There are some existing attributes that similarly affect semantics > in ways that cannot be ignored. vector_size is one obvious example. > But that doesn't make it a good thing. :) > > Also, C++ says this for standard [[...]] attributes: > > For an attribute-token (including an attribute-scoped-token) > not specified in this document, the behavior is implementation-defined; > any such attribute-token that is not recognized by the implementation > is ignored. > > which doubles down on the idea that attributes should not be used > for necessary semantic information. > > One of the attributes we'd like to add provides a new way of compiling > existing code. The attribute doesn't require SME to be available; > it just says that the code must be compiled so that it can run in either > of two modes. This is probably the most dangerous attribute of the set, > since compilers that ignore it would just produce normal code. That > code might work in some test scenarios, but it would fail in others. > > The feeling from the Clang community was therefore that these SME > attributes should use keywords instead, so that the keywords trigger > an error with older compilers. > > However, it seemed wrong to define new SME-specific grammar rules, > since the underlying problem is pretty generic. We therefore > proposed having a type of keyword that can appear exactly where > a standard [[...]] attribute can appear and that appertains to > exactly what a standard [[...]] attribute would appertain to. > No divergence or cherry-picking is allowed. > > For example: > > [[arm::foo]] > > would become: > > __arm_foo > > and: > > [[arm::bar(args)]] > > would become: > > __arm_bar(args) > > It wouldn't be possible to retrofit arguments to a keyword that > previously didn't take arguments, since that could lead to parsing > ambiguities. So when a keyword is first added, a binding decision > would need to be made whether the keyword always takes arguments > or is always standalone. > > For that reason, empty argument lists are allowed for keywords, > even though they're not allowed for [[...]] attributes. > > The argument-less version was accepted into Clang, and I have a follow-on > patch for handling arguments. Would the same thing be OK for GCC, > in both the C and C++ frontends? > > The patch below is a proof of concept for the C frontend. It doesn't > bootstrap due to warnings about uninitialised fields. And it doesn't > have tests. But I did test it locally with various combinations of > attribute_spec and it seemed to work as expected. > > The impact on the C frontend seems to be pretty small. It looks like > the impact on the C++ frontend would be a bit bigger, but not much. > > The patch contains a logically unrelated change: c-common.h set aside > 16 keywords for address spaces, but of the in-tree ports, the maximum > number of keywords used is 6 (for amdgcn). The patch therefore changes > the limit to 8 and uses 8 keywords for the new attributes. This keeps > the number of reserved ids <= 256.
If you had added __arm(bar(args)) instead of __arm_bar(args) you would only need one additional keyword - we could set aside a similar one for each target then. I realize that double-nesting of arguments might prove a bit challenging but still. In any case I also think that attributes are what you want and their ugliness/issues are not worse than the ugliness/issues of the keyword approach IMHO. Richard. > A real, non-proof-of-concept patch series would: > > - Change the address-space keywords separately, and deal with any fallout. > > - Clean up the way that attributes are specified, so that it isn't > necessary to update all definitions when adding a new field. > > - Allow more precise attribute requirements, such as "function decl only". > > - Add tests :) > > WDYT? Does this approach look OK in principle, or is it a non-starter? > > If it is a non-starter, the fallback would be to predefine macros > that expand to [[...]] or __attribute__. Having the keywords gives > more precise semantics and better error messages though. > > Thanks, > Richard > --- > gcc/attribs.cc | 30 +++++++++++- > gcc/c-family/c-common.h | 13 ++---- > gcc/c/c-parser.cc | 88 +++++++++++++++++++++++++++++++++-- > gcc/config/aarch64/aarch64.cc | 1 + > gcc/tree-core.h | 19 ++++++++ > 5 files changed, 135 insertions(+), 16 deletions(-) > > diff --git a/gcc/attribs.cc b/gcc/attribs.cc > index b8cb55b97df..706cd81329c 100644 > --- a/gcc/attribs.cc > +++ b/gcc/attribs.cc > @@ -752,6 +752,11 @@ decl_attributes (tree *node, tree attributes, int flags, > > if (spec->decl_required && !DECL_P (*anode)) > { > + if (spec->is_keyword) > + { > + error ("%qE does not apply to types", name); > + continue; > + } > if (flags & ((int) ATTR_FLAG_DECL_NEXT > | (int) ATTR_FLAG_FUNCTION_NEXT > | (int) ATTR_FLAG_ARRAY_NEXT)) > @@ -775,6 +780,11 @@ decl_attributes (tree *node, tree attributes, int flags, > the decl's type in place here. */ > if (spec->type_required && DECL_P (*anode)) > { > + if (spec->is_keyword) > + { > + error ("%qE only applies to types", name); > + continue; > + } > anode = &TREE_TYPE (*anode); > flags &= ~(int) ATTR_FLAG_TYPE_IN_PLACE; > } > @@ -782,6 +792,11 @@ decl_attributes (tree *node, tree attributes, int flags, > if (spec->function_type_required && TREE_CODE (*anode) != FUNCTION_TYPE > && TREE_CODE (*anode) != METHOD_TYPE) > { > + if (spec->is_keyword) > + { > + error ("%qE only applies to function types", name); > + continue; > + } > if (TREE_CODE (*anode) == POINTER_TYPE > && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (*anode))) > { > @@ -821,7 +836,12 @@ decl_attributes (tree *node, tree attributes, int flags, > && (flags & (int) ATTR_FLAG_TYPE_IN_PLACE) > && COMPLETE_TYPE_P (*anode)) > { > - warning (OPT_Wattributes, "type attributes ignored after type is > already defined"); > + if (spec->is_keyword) > + error ("cannot apply %qE to a type that has already been" > + " defined", name); > + else > + warning (OPT_Wattributes, "type attributes ignored after type" > + " is already defined"); > continue; > } > > @@ -895,7 +915,13 @@ decl_attributes (tree *node, tree attributes, int flags, > *anode = cur_and_last_decl[0]; > if (ret == error_mark_node) > { > - warning (OPT_Wattributes, "%qE attribute ignored", name); > + if (spec->is_keyword) > + /* This error is only a last resort, Hopefully the > + handler will report a better error in most cases, > + return NULL_TREE, and set no_add_attrs. */ > + error ("invalid %qE attribute", name); > + else > + warning (OPT_Wattributes, "%qE attribute ignored", name); > no_add_attrs = true; > } > else > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h > index b5ef5ff6b2c..4443ccb874d 100644 > --- a/gcc/c-family/c-common.h > +++ b/gcc/c-family/c-common.h > @@ -222,17 +222,9 @@ enum rid > RID_ADDR_SPACE_5, > RID_ADDR_SPACE_6, > RID_ADDR_SPACE_7, > - RID_ADDR_SPACE_8, > - RID_ADDR_SPACE_9, > - RID_ADDR_SPACE_10, > - RID_ADDR_SPACE_11, > - RID_ADDR_SPACE_12, > - RID_ADDR_SPACE_13, > - RID_ADDR_SPACE_14, > - RID_ADDR_SPACE_15, > > RID_FIRST_ADDR_SPACE = RID_ADDR_SPACE_0, > - RID_LAST_ADDR_SPACE = RID_ADDR_SPACE_15, > + RID_LAST_ADDR_SPACE = RID_ADDR_SPACE_7, > > /* __intN keywords. The _N_M here doesn't correspond to the intN > in the keyword; use the bitsize in int_n_t_data_t[M] for that. > @@ -251,6 +243,9 @@ enum rid > RID_FIRST_INT_N = RID_INT_N_0, > RID_LAST_INT_N = RID_INT_N_3, > > + RID_FIRST_KEYWORD_ATTR, > + RID_LAST_KEYWORD_ATTR = RID_FIRST_KEYWORD_ATTR + 7, > + > RID_MAX, > > RID_FIRST_MODIFIER = RID_STATIC, > diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc > index 24a6eb6e459..b02b082b140 100644 > --- a/gcc/c/c-parser.cc > +++ b/gcc/c/c-parser.cc > @@ -104,6 +104,17 @@ set_c_expr_source_range (c_expr *expr, > } > > > +/* Register keyword attribute AS with reserved identifier code RID. */ > + > +void > +c_register_keyword_attribute (const attribute_spec *as, int rid) > +{ > + tree id = get_identifier (as->name); > + C_SET_RID_CODE (id, rid); > + C_IS_RESERVED_WORD (id) = 1; > + ridpointers [rid] = id; > +} > + > /* Initialization routine for this file. */ > > void > @@ -180,6 +191,16 @@ c_parse_init (void) > C_IS_RESERVED_WORD (id) = 1; > ridpointers [RID_OMP_ALL_MEMORY] = id; > } > + > + int rid = RID_FIRST_KEYWORD_ATTR; > + if (const attribute_spec *attrs = targetm.attribute_table) > + for (const attribute_spec *attr = attrs; attr->name; ++attr) > + if (attr->is_keyword) > + { > + gcc_assert (rid <= RID_LAST_KEYWORD_ATTR); > + c_register_keyword_attribute (attr, rid); > + rid += 1; > + } > } > > /* A parser structure recording information about the state and > @@ -4951,7 +4972,8 @@ c_parser_gnu_attribute_any_word (c_parser *parser) > { > /* ??? See comment above about what keywords are accepted here. */ > bool ok; > - switch (c_parser_peek_token (parser)->keyword) > + int rid = c_parser_peek_token (parser)->keyword; > + switch (rid) > { > case RID_STATIC: > case RID_UNSIGNED: > @@ -4994,7 +5016,9 @@ c_parser_gnu_attribute_any_word (c_parser *parser) > ok = true; > break; > default: > - ok = false; > + /* Accept these now so that we can reject them with a specific > + error later. */ > + ok = (rid >= RID_FIRST_KEYWORD_ATTR && rid <= > RID_LAST_KEYWORD_ATTR); > break; > } > if (!ok) > @@ -5156,6 +5180,10 @@ c_parser_gnu_attribute (c_parser *parser, tree attrs, > return NULL_TREE; > > attr_name = canonicalize_attr_name (attr_name); > + const attribute_spec *as = lookup_attribute_spec (attr_name); > + if (as && as->is_keyword) > + error ("%qE cannot be used in %<__attribute__%> constructs", attr_name); > + > c_parser_consume_token (parser); > > tree attr; > @@ -5330,6 +5358,42 @@ c_parser_balanced_token_sequence (c_parser *parser) > } > } > > +/* Parse a keyword attribute. This is simply: > + > + keyword > + > + if the attribute never takes arguments, otherwise it is: > + > + keyword ( balanced-token-sequence[opt] ) > +*/ > + > +static tree > +c_parser_keyword_attribute (c_parser *parser) > +{ > + c_token *token = c_parser_peek_token (parser); > + gcc_assert (token->type == CPP_KEYWORD); > + tree name = canonicalize_attr_name (token->value); > + c_parser_consume_token (parser); > + > + tree attribute = build_tree_list (name, NULL_TREE); > + const attribute_spec *as = lookup_attribute_spec (name); > + gcc_assert (as && as->is_keyword); > + if (as->max_length > 0) > + { > + matching_parens parens; > + if (!parens.require_open (parser)) > + return error_mark_node; > + /* Allow zero-length arguments regardless of as->min_length, since > + the usual error "parentheses must be omitted if attribute argument > + list is empty" suggests an invalid change. We'll reject incorrect > + argument lists later. */ > + TREE_VALUE (attribute) > + = c_parser_attribute_arguments (parser, false, false, false, true); > + parens.require_close (parser); > + } > + return attribute; > +} > + > /* Parse standard (C2X) attributes (including GNU attributes in the > gnu:: namespace). > > @@ -5396,8 +5460,11 @@ c_parser_std_attribute (c_parser *parser, bool for_tm) > ns = NULL_TREE; > attribute = build_tree_list (build_tree_list (ns, name), NULL_TREE); > > - /* Parse the arguments, if any. */ > const attribute_spec *as = lookup_attribute_spec (TREE_PURPOSE > (attribute)); > + if (as && as->is_keyword) > + error ("%qE cannot be used in %<[[...]]%> constructs", name); > + > + /* Parse the arguments, if any. */ > if (c_parser_next_token_is_not (parser, CPP_OPEN_PAREN)) > goto out; > { > @@ -5456,7 +5523,13 @@ c_parser_std_attribute (c_parser *parser, bool for_tm) > static tree > c_parser_std_attribute_specifier (c_parser *parser, bool for_tm) > { > - location_t loc = c_parser_peek_token (parser)->location; > + auto first_token = c_parser_peek_token (parser); > + location_t loc = first_token->location; > + if (first_token->type == CPP_KEYWORD > + && first_token->keyword >= RID_FIRST_KEYWORD_ATTR > + && first_token->keyword <= RID_LAST_KEYWORD_ATTR) > + return c_parser_keyword_attribute (parser); > + > if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>")) > return NULL_TREE; > if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>")) > @@ -5571,7 +5644,12 @@ c_parser_check_balanced_raw_token_sequence (c_parser > *parser, unsigned int *n) > static bool > c_parser_nth_token_starts_std_attributes (c_parser *parser, unsigned int n) > { > - if (!(c_parser_peek_nth_token (parser, n)->type == CPP_OPEN_SQUARE > + auto token_n = c_parser_peek_nth_token (parser, n); > + if (token_n->type == CPP_KEYWORD > + && token_n->keyword >= RID_FIRST_KEYWORD_ATTR > + && token_n->keyword <= RID_LAST_KEYWORD_ATTR) > + return true; > + if (!(token_n->type == CPP_OPEN_SQUARE > && c_parser_peek_nth_token (parser, n + 1)->type == CPP_OPEN_SQUARE)) > return false; > /* In C, '[[' must start attributes. In Objective-C, we need to > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc > index 560e5431636..50698439104 100644 > --- a/gcc/config/aarch64/aarch64.cc > +++ b/gcc/config/aarch64/aarch64.cc > @@ -2802,6 +2802,7 @@ static const struct attribute_spec > aarch64_attribute_table[] = > { "arm_sve_vector_bits", 1, 1, false, true, false, true, > aarch64_sve::handle_arm_sve_vector_bits_attribute, > NULL }, > + { "__arm_streaming", 0, 0, false, true, true, true, NULL, NULL, true > }, > { "Advanced SIMD type", 1, 1, false, true, false, true, NULL, NULL }, > { "SVE type", 3, 3, false, true, false, true, NULL, > NULL }, > { "SVE sizeless type", 0, 0, false, true, false, true, NULL, NULL }, > diff --git a/gcc/tree-core.h b/gcc/tree-core.h > index 668808a29d0..e6700509b9e 100644 > --- a/gcc/tree-core.h > +++ b/gcc/tree-core.h > @@ -2167,6 +2167,25 @@ struct attribute_spec { > /* An array of attribute exclusions describing names of other attributes > that this attribute is mutually exclusive with. */ > const exclusions *exclude; > + > + /* Whether the attribute is a C/C++ "regular keyword attribute". > + When true for an attribute "foo", this means that: > + > + - The keyword foo can appear exactly where the standard attribute syntax > + [[...]] can appear, but without the same minimum language > requirements. > + The link is intended to be automatic: there should be no exceptions. > + > + - The attribute appertains to whatever a standard attribute in the > + same location would appertain to. There is no "sliding" from decls > + to types, as sometimes happens for GNU-style attributes. > + > + - When MAX_LENGTH > 0, the keyword is followed by an argument list. > + This argument list is parsed in the same way as arguments to [[...]] > + attributes, except that the list can be empty if MIN_LENGTH == 0. > + > + - In C and C++, the attribute cannot appear in __attribute__ or [[...]]; > + it can only appear as a simple keyword. */ > + bool is_keyword; > }; > > /* These functions allow a front-end to perform a manual layout of a > -- > 2.25.1 >