From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x530.google.com (mail-pg1-x530.google.com [IPv6:2607:f8b0:4864:20::530]) by sourceware.org (Postfix) with ESMTPS id 7A0C23858D1E for ; Mon, 17 Jul 2023 06:38:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7A0C23858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pg1-x530.google.com with SMTP id 41be03b00d2f7-55ae51a45deso2513933a12.3 for ; Sun, 16 Jul 2023 23:38:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1689575929; x=1692167929; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=A00WfG8i2z4JDtpQkX35JIIKryvyJ7vOG8WoneSX3dc=; b=Ger3VLOwKxRqRdWvF4AbJh7w5IvTfL4RrTUVTItGI218U9eG4FmLrbDYU/C3T+okng AjBWR7NkI4BIKzv7cTEEoWal35cXs6JsrKWBbZZzpKKxp41GKgrGHb/eIf3yUXG7tqd+ vJqtWdz5v7nm/84xsfh9ffxuXCzlzNVuvTu6Cb8fzAeJUGrGqmRqo4Fzgbmn8L0Tql8r qBr9eHHuRX6u2R9bLvhD2r92YpXrWDYeP6BTiqgd2xIy9BjJ+MZVFiq1A+InJMO1Yclf Am9FTOsmjlwVvVe4DDS0dk4HVrrpspEt+NaatrAoNXS0mHW/N+p5v65E+5pwE9jaZQD6 uBeg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689575929; x=1692167929; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=A00WfG8i2z4JDtpQkX35JIIKryvyJ7vOG8WoneSX3dc=; b=fUO60v2XJQ4MESnstVfO5zQmiuS/A5qegvuWBe9rcpGLLi9gKdlU9jnmQIVty8lQLt cpadGHYaU4gE2bRnTAxVmrm2aQ9LoQvdTuToQQD3Cb7vAVpEY99MHdsWQxEorGeiygYP vDQMd0l/DXlBdV4bqms45kJfWj358RS+iVAzVQkf4DKi4k6xMIgXSzjrGho9EyaCteUS gDHhCe+CBIZsqRajLvyRy/nns+XKt/dgFSjZdjgqFwkcUw8RNaWMJ+7CbvAQQNvcU5Vj qcDAfsPUxHfmkcHVkx77G+v+gReuH+NMwUaQ8ViMjZXmukFMIIj5G7Gu9oPw5/F+Y8tu fbYg== X-Gm-Message-State: ABy/qLYqVnYT2ZYkpBZcGWgonmyDvu0klvWtOvabSSUWTwccKsHoFi8E y6CMptwdWdmyWKKvc59xNo0mHFSZr6ayyDl080I= X-Google-Smtp-Source: APBJJlGCIMzCY5nzdNSsjAvk90EF8ZwQrfFlHaiCH2oq7/i5L9sX3PF/NqtWv3z5qOcb6M5GcC5lCzqb0wzl5NoVU+c= X-Received: by 2002:a17:90a:cb03:b0:263:ac11:c6d2 with SMTP id z3-20020a17090acb0300b00263ac11c6d2mr9510743pjt.25.1689575929181; Sun, 16 Jul 2023 23:38:49 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Mon, 17 Jul 2023 08:38:15 +0200 Message-ID: Subject: Re: [WIP RFC] Add support for keyword-based attributes To: Richard Sandiford , gcc-patches@gcc.gnu.org, joseph@codesourcery.com, polacek@redhat.com, jason@redhat.com, nathan@acm.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, Jul 14, 2023 at 5:58=E2=80=AFPM Richard Sandiford via Gcc-patches 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 <=3D 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 fl= ags, > > 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 fl= ags, > 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 =3D &TREE_TYPE (*anode); > flags &=3D ~(int) ATTR_FLAG_TYPE_IN_PLACE; > } > @@ -782,6 +792,11 @@ decl_attributes (tree *node, tree attributes, int fl= ags, > if (spec->function_type_required && TREE_CODE (*anode) !=3D FUNCTI= ON_TYPE > && TREE_CODE (*anode) !=3D METHOD_TYPE) > { > + if (spec->is_keyword) > + { > + error ("%qE only applies to function types", name); > + continue; > + } > if (TREE_CODE (*anode) =3D=3D POINTER_TYPE > && FUNC_OR_METHOD_TYPE_P (TREE_TYPE (*anode))) > { > @@ -821,7 +836,12 @@ decl_attributes (tree *node, tree attributes, int fl= ags, > && (flags & (int) ATTR_FLAG_TYPE_IN_PLACE) > && COMPLETE_TYPE_P (*anode)) > { > - warning (OPT_Wattributes, "type attributes ignored after type i= s 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 fl= ags, > *anode =3D cur_and_last_decl[0]; > if (ret =3D=3D 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 =3D 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 =3D RID_ADDR_SPACE_0, > - RID_LAST_ADDR_SPACE =3D RID_ADDR_SPACE_15, > + RID_LAST_ADDR_SPACE =3D 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 =3D RID_INT_N_0, > RID_LAST_INT_N =3D RID_INT_N_3, > > + RID_FIRST_KEYWORD_ATTR, > + RID_LAST_KEYWORD_ATTR =3D RID_FIRST_KEYWORD_ATTR + 7, > + > RID_MAX, > > RID_FIRST_MODIFIER =3D 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 =3D get_identifier (as->name); > + C_SET_RID_CODE (id, rid); > + C_IS_RESERVED_WORD (id) =3D 1; > + ridpointers [rid] =3D id; > +} > + > /* Initialization routine for this file. */ > > void > @@ -180,6 +191,16 @@ c_parse_init (void) > C_IS_RESERVED_WORD (id) =3D 1; > ridpointers [RID_OMP_ALL_MEMORY] =3D id; > } > + > + int rid =3D RID_FIRST_KEYWORD_ATTR; > + if (const attribute_spec *attrs =3D targetm.attribute_table) > + for (const attribute_spec *attr =3D attrs; attr->name; ++attr) > + if (attr->is_keyword) > + { > + gcc_assert (rid <=3D RID_LAST_KEYWORD_ATTR); > + c_register_keyword_attribute (attr, rid); > + rid +=3D 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 =3D 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 =3D true; > break; > default: > - ok =3D false; > + /* Accept these now so that we can reject them with a specific > + error later. */ > + ok =3D (rid >=3D RID_FIRST_KEYWORD_ATTR && rid <=3D RID_LAST_KE= YWORD_ATTR); > break; > } > if (!ok) > @@ -5156,6 +5180,10 @@ c_parser_gnu_attribute (c_parser *parser, tree att= rs, > return NULL_TREE; > > attr_name =3D canonicalize_attr_name (attr_name); > + const attribute_spec *as =3D lookup_attribute_spec (attr_name); > + if (as && as->is_keyword) > + error ("%qE cannot be used in %<__attribute__%> constructs", attr_na= me); > + > 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 =3D c_parser_peek_token (parser); > + gcc_assert (token->type =3D=3D CPP_KEYWORD); > + tree name =3D canonicalize_attr_name (token->value); > + c_parser_consume_token (parser); > + > + tree attribute =3D build_tree_list (name, NULL_TREE); > + const attribute_spec *as =3D 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 argume= nt > + list is empty" suggests an invalid change. We'll reject incorre= ct > + argument lists later. */ > + TREE_VALUE (attribute) > + =3D c_parser_attribute_arguments (parser, false, false, false, tr= ue); > + 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 =3D NULL_TREE; > attribute =3D build_tree_list (build_tree_list (ns, name), NULL_TREE); > > - /* Parse the arguments, if any. */ > const attribute_spec *as =3D lookup_attribute_spec (TREE_PURPOSE (attr= ibute)); > + 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 =3D c_parser_peek_token (parser)->location; > + auto first_token =3D c_parser_peek_token (parser); > + location_t loc =3D first_token->location; > + if (first_token->type =3D=3D CPP_KEYWORD > + && first_token->keyword >=3D RID_FIRST_KEYWORD_ATTR > + && first_token->keyword <=3D 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_pars= er *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 =3D=3D CPP_OPEN_SQUARE > + auto token_n =3D c_parser_peek_nth_token (parser, n); > + if (token_n->type =3D=3D CPP_KEYWORD > + && token_n->keyword >=3D RID_FIRST_KEYWORD_ATTR > + && token_n->keyword <=3D RID_LAST_KEYWORD_ATTR) > + return true; > + if (!(token_n->type =3D=3D CPP_OPEN_SQUARE > && c_parser_peek_nth_token (parser, n + 1)->type =3D=3D 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.c= c > 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_attribut= e_table[] =3D > { "arm_sve_vector_bits", 1, 1, false, true, false, true, > aarch64_sve::handle_arm_sve_vector_bits_attribu= te, > 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 attribut= es > 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 s= yntax > + [[...]] can appear, but without the same minimum language require= ments. > + The link is intended to be automatic: there should be no exceptio= ns. > + > + - The attribute appertains to whatever a standard attribute in the > + same location would appertain to. There is no "sliding" from dec= ls > + 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 =3D= =3D 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 >