* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-14 15:56 [WIP RFC] Add support for keyword-based attributes Richard Sandiford
@ 2023-07-14 17:17 ` Jakub Jelinek
2023-07-16 10:50 ` Richard Sandiford
2023-07-14 21:14 ` Nathan Sidwell
2023-07-17 6:38 ` Richard Biener
2 siblings, 1 reply; 19+ messages in thread
From: Jakub Jelinek @ 2023-07-14 17:17 UTC (permalink / raw)
To: gcc-patches, joseph, polacek, jason, nathan, richard.sandiford
On Fri, Jul 14, 2023 at 04:56:18PM +0100, 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?
Will defer to C/C++ maintainers, but as you mentioned, there are many
attributes which really can't be ignored and change behavior significantly.
vector_size is one of those, mode attribute another,
no_unique_address another one (changes ABI in various cases),
the OpenMP attributes (omp::directive, omp::sequence) can change
behavior if -fopenmp, etc.
One can easily error with
#ifdef __has_cpp_attribute
#if !__has_cpp_attribute (arm::whatever)
#error arm::whatever attribute unsupported
#endif
#else
#error __has_cpp_attribute unsupported
#endif
Adding keywords instead of attributes seems to be too ugly to me.
Jakub
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-14 17:17 ` Jakub Jelinek
@ 2023-07-16 10:50 ` Richard Sandiford
2023-07-17 13:39 ` Jason Merrill
0 siblings, 1 reply; 19+ messages in thread
From: Richard Sandiford @ 2023-07-16 10:50 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, joseph, polacek, jason, nathan
Jakub Jelinek <jakub@redhat.com> writes:
> On Fri, Jul 14, 2023 at 04:56:18PM +0100, 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?
>
> Will defer to C/C++ maintainers, but as you mentioned, there are many
> attributes which really can't be ignored and change behavior significantly.
> vector_size is one of those, mode attribute another,
> no_unique_address another one (changes ABI in various cases),
> the OpenMP attributes (omp::directive, omp::sequence) can change
> behavior if -fopenmp, etc.
> One can easily error with
> #ifdef __has_cpp_attribute
> #if !__has_cpp_attribute (arm::whatever)
> #error arm::whatever attribute unsupported
> #endif
> #else
> #error __has_cpp_attribute unsupported
> #endif
Yeah. It's easy to detect whether a particular ACLE feature is supported,
since there are predefined macros for each one. But IMO it's a failing
if we have to recommend that any compilation that uses arm::foo should
also have:
#ifndef __ARM_FEATURE_FOO
#error arm::foo not supported
#endif
It ought to be the compiler's job to diagnose its limitations, rather
than the user's.
The feature macros are more for conditional usage of features, where
there's a fallback available.
I suppose we could say that users have to include a particular header
file before using an attribute, and use a pragma in that header file to
tell the compiler to enable the attribute. But then there would need to
be a separate header file for each distinct set of attributes (in terms
of historical timeline), which would get ugly. I'm not sure that it's
better than using keywords, or even whether it's better than predefining
the "keyword" as a macro that expands to a compiler-internal attribute.
Thanks,
Richard
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-16 10:50 ` Richard Sandiford
@ 2023-07-17 13:39 ` Jason Merrill
2023-07-17 14:06 ` Richard Sandiford
0 siblings, 1 reply; 19+ messages in thread
From: Jason Merrill @ 2023-07-17 13:39 UTC (permalink / raw)
To: Jakub Jelinek, gcc-patches, joseph, polacek, jason, nathan,
richard.sandiford
[-- Attachment #1: Type: text/plain, Size: 2250 bytes --]
On Sun, Jul 16, 2023 at 6:50 AM Richard Sandiford <richard.sandiford@arm.com>
wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> > On Fri, Jul 14, 2023 at 04:56:18PM +0100, 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?
> >
> > Will defer to C/C++ maintainers, but as you mentioned, there are many
> > attributes which really can't be ignored and change behavior
> significantly.
> > vector_size is one of those, mode attribute another,
> > no_unique_address another one (changes ABI in various cases),
> > the OpenMP attributes (omp::directive, omp::sequence) can change
> > behavior if -fopenmp, etc.
> > One can easily error with
> > #ifdef __has_cpp_attribute
> > #if !__has_cpp_attribute (arm::whatever)
> > #error arm::whatever attribute unsupported
> > #endif
> > #else
> > #error __has_cpp_attribute unsupported
> > #endif
>
> Yeah. It's easy to detect whether a particular ACLE feature is supported,
> since there are predefined macros for each one. But IMO it's a failing
> if we have to recommend that any compilation that uses arm::foo should
> also have:
>
> #ifndef __ARM_FEATURE_FOO
> #error arm::foo not supported
> #endif
>
> It ought to be the compiler's job to diagnose its limitations, rather
> than the user's.
>
> The feature macros are more for conditional usage of features, where
> there's a fallback available.
>
> I suppose we could say that users have to include a particular header
> file before using an attribute, and use a pragma in that header file to
> tell the compiler to enable the attribute. But then there would need to
> be a separate header file for each distinct set of attributes (in terms
> of historical timeline), which would get ugly. I'm not sure that it's
> better than using keywords, or even whether it's better than predefining
> the "keyword" as a macro that expands to a compiler-internal attribute.
>
With a combination of those approaches it can be a single header:
#ifdef __ARM_FEATURE_FOO
#define __arm_foo [[arm::foo]]
// else use of __arm_foo will fail
#endif
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-17 13:39 ` Jason Merrill
@ 2023-07-17 14:06 ` Richard Sandiford
0 siblings, 0 replies; 19+ messages in thread
From: Richard Sandiford @ 2023-07-17 14:06 UTC (permalink / raw)
To: Jason Merrill; +Cc: Jakub Jelinek, gcc-patches, joseph, polacek, nathan
Jason Merrill <jason@redhat.com> writes:
> On Sun, Jul 16, 2023 at 6:50 AM Richard Sandiford <richard.sandiford@arm.com>
> wrote:
>
>> Jakub Jelinek <jakub@redhat.com> writes:
>> > On Fri, Jul 14, 2023 at 04:56:18PM +0100, 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?
>> >
>> > Will defer to C/C++ maintainers, but as you mentioned, there are many
>> > attributes which really can't be ignored and change behavior
>> significantly.
>> > vector_size is one of those, mode attribute another,
>> > no_unique_address another one (changes ABI in various cases),
>> > the OpenMP attributes (omp::directive, omp::sequence) can change
>> > behavior if -fopenmp, etc.
>> > One can easily error with
>> > #ifdef __has_cpp_attribute
>> > #if !__has_cpp_attribute (arm::whatever)
>> > #error arm::whatever attribute unsupported
>> > #endif
>> > #else
>> > #error __has_cpp_attribute unsupported
>> > #endif
>>
>> Yeah. It's easy to detect whether a particular ACLE feature is supported,
>> since there are predefined macros for each one. But IMO it's a failing
>> if we have to recommend that any compilation that uses arm::foo should
>> also have:
>>
>> #ifndef __ARM_FEATURE_FOO
>> #error arm::foo not supported
>> #endif
>>
>> It ought to be the compiler's job to diagnose its limitations, rather
>> than the user's.
>>
>> The feature macros are more for conditional usage of features, where
>> there's a fallback available.
>>
>> I suppose we could say that users have to include a particular header
>> file before using an attribute, and use a pragma in that header file to
>> tell the compiler to enable the attribute. But then there would need to
>> be a separate header file for each distinct set of attributes (in terms
>> of historical timeline), which would get ugly. I'm not sure that it's
>> better than using keywords, or even whether it's better than predefining
>> the "keyword" as a macro that expands to a compiler-internal attribute.
>>
>
> With a combination of those approaches it can be a single header:
>
> #ifdef __ARM_FEATURE_FOO
> #define __arm_foo [[arm::foo]]
> // else use of __arm_foo will fail
> #endif
If we did that, would it be a defined part of the interface that
__arm_foo expands to exactly arm::foo, rather than to an obfuscated
or compiler-dependent attribute?
In other words, would it be a case of providing both the attribute
and the macro, and leaving users to choose whether they use the
attribute directly (and run the risk of miscompilation) or whether
they use the macros, based on their risk appetite? If so, the risk of
miscompliation is mostly borne by the people who build the deployed code
rather than the people who initially wrote it.
If instead we say that the expansion of the macros is compiler-dependent
and that the macros must always be used, then I'm not sure the header
file provides a better interface than predefining the macros in the
compiler (which was the fallback option if the keywords were rejected).
But the diagnostics using these macros would be worse than diagnostics
based on keywords, not least because the diagnostics about invalid
use of the macros (from compilers that understood them) would refer
to the underlying attribute rather than the macro.
Thanks,
Richard
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-14 15:56 [WIP RFC] Add support for keyword-based attributes Richard Sandiford
2023-07-14 17:17 ` Jakub Jelinek
@ 2023-07-14 21:14 ` Nathan Sidwell
2023-07-16 10:18 ` Richard Sandiford
2023-07-17 6:38 ` Richard Biener
2 siblings, 1 reply; 19+ messages in thread
From: Nathan Sidwell @ 2023-07-14 21:14 UTC (permalink / raw)
To: gcc-patches, joseph, polacek, jason, richard.sandiford
On 7/14/23 11:56, Richard Sandiford 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.
There;s been quite a bit of discussion about the practicalities of that. As you
say, there are existing, std-specified attributes, [[no_unique_address]] for
instance, that affect user-visible object layout if ignored.
Further, my understanding is that implementation-specific attributes are
permitted to affect program semantics -- they're implementatin extensions.
IMHO, attributes are the accepted mechanism for what you're describing.
Compilers already have a way of dealing with them -- both parsing and, in
general, representing them. I would be wary of inventing a different mechanism.
Have you approached C or C++ std bodies for input?
>
> 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.
>
> 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,
> }
>
> \f
> +/* 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;
> + }
> }
> \f
> /* 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
--
Nathan Sidwell
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-14 21:14 ` Nathan Sidwell
@ 2023-07-16 10:18 ` Richard Sandiford
0 siblings, 0 replies; 19+ messages in thread
From: Richard Sandiford @ 2023-07-16 10:18 UTC (permalink / raw)
To: Nathan Sidwell; +Cc: gcc-patches, joseph, polacek, jason
Thanks for the feedback.
Nathan Sidwell <nathan@acm.org> writes:
> On 7/14/23 11:56, Richard Sandiford 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.
>
> There;s been quite a bit of discussion about the practicalities of that. As you
> say, there are existing, std-specified attributes, [[no_unique_address]] for
> instance, that affect user-visible object layout if ignored.
> Further, my understanding is that implementation-specific attributes are
> permitted to affect program semantics -- they're implementatin extensions.
>
> IMHO, attributes are the accepted mechanism for what you're describing.
> Compilers already have a way of dealing with them -- both parsing and, in
> general, representing them. I would be wary of inventing a different mechanism.
>
> Have you approached C or C++ std bodies for input?
Not directly, although members of the C++ WG contributed to the discussion
on the Clang side.
But I'm not sure how much the WG can help. The main constraint here is
what existing compilers do, and that can't be changed. Similarly, any
future Arm extension that hasn't been invented yet might need new semantic
attributes, and those attributes would be ignored by GCC 14, etc.
Thanks,
Richard
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-14 15:56 [WIP RFC] Add support for keyword-based attributes Richard Sandiford
2023-07-14 17:17 ` Jakub Jelinek
2023-07-14 21:14 ` Nathan Sidwell
@ 2023-07-17 6:38 ` Richard Biener
2023-07-17 8:21 ` Richard Sandiford
2 siblings, 1 reply; 19+ messages in thread
From: Richard Biener @ 2023-07-17 6:38 UTC (permalink / raw)
To: Richard Sandiford, gcc-patches, joseph, polacek, jason, nathan
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-17 6:38 ` Richard Biener
@ 2023-07-17 8:21 ` Richard Sandiford
2023-07-17 9:05 ` Richard Biener
2023-07-17 13:53 ` Michael Matz
0 siblings, 2 replies; 19+ messages in thread
From: Richard Sandiford @ 2023-07-17 8:21 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, joseph, polacek, jason, nathan
Richard Biener <richard.guenther@gmail.com> writes:
> 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.
Yeah, that would work.
> 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.
I guess the ugliness of keywords is the double underscore?
What are the issues with the keyword approach though?
If it's two underscores vs miscompilation then it's not obvious
that two underscores should lose.
Richard
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-17 8:21 ` Richard Sandiford
@ 2023-07-17 9:05 ` Richard Biener
2023-07-17 13:53 ` Michael Matz
1 sibling, 0 replies; 19+ messages in thread
From: Richard Biener @ 2023-07-17 9:05 UTC (permalink / raw)
To: Richard Biener, gcc-patches, joseph, polacek, jason, nathan,
richard.sandiford
On Mon, Jul 17, 2023 at 10:21 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Richard Biener <richard.guenther@gmail.com> writes:
> > 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.
>
> Yeah, that would work.
>
> > 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.
>
> I guess the ugliness of keywords is the double underscore?
> What are the issues with the keyword approach though?
The issue is the non-standard syntax which will confuse 3rd party
tools like IDEs, static analyzers, etc. I'd also add that esp.
my suggestion to use __arm will likely clash with pre-existing
macros from (some) implementations. That can be solved with
_ArmKWD or choosing a less "common" identifier. A quick
check shows GCC on arm only defines __arm__, not __arm though.
Richard.
> If it's two underscores vs miscompilation then it's not obvious
> that two underscores should lose.
>
> Richard
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-17 8:21 ` Richard Sandiford
2023-07-17 9:05 ` Richard Biener
@ 2023-07-17 13:53 ` Michael Matz
2023-07-21 23:25 ` Joseph Myers
1 sibling, 1 reply; 19+ messages in thread
From: Michael Matz @ 2023-07-17 13:53 UTC (permalink / raw)
To: Richard Sandiford
Cc: Richard Biener, gcc-patches, joseph, polacek, jason, nathan
Hello,
On Mon, 17 Jul 2023, Richard Sandiford via Gcc-patches wrote:
> >> 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. :)
> >...
> > 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.
>
> Yeah, that would work.
So, essentially you want unignorable attributes, right? Then implement
exactly that: add one new keyword "__known_attribute__" (invent a better
name, maybe :) ), semantics exactly as with __attribute__ (including using
the same underlying lists in our data structures), with only one single
deviation: instead of the warning you give an error for unhandled
attributes. Done.
(Old compilers will barf of the unknown new keyword, new compilers will
error on unknown values used within such attribute list)
> > 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.
>
> I guess the ugliness of keywords is the double underscore? What are the
> issues with the keyword approach though?
There are _always_ problems with new keywords, the more new keywords the
more problems :-) Is the keyword context-sensitive or not? What about
existing system headers that use it right now? Is it recognized in
free-standing or not? Is it only recognized for some targets? Is it
recognized only for certain configurations of the target?
So, let's define one new mechanism, for all targets, all configs, and all
language standards. Let's use __attribute__ with a twist :)
Ciao,
Michael.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-17 13:53 ` Michael Matz
@ 2023-07-21 23:25 ` Joseph Myers
2023-08-16 10:36 ` Richard Sandiford
0 siblings, 1 reply; 19+ messages in thread
From: Joseph Myers @ 2023-07-21 23:25 UTC (permalink / raw)
To: Michael Matz
Cc: Richard Sandiford, Richard Biener, gcc-patches, polacek, jason, nathan
On Mon, 17 Jul 2023, Michael Matz via Gcc-patches wrote:
> So, essentially you want unignorable attributes, right? Then implement
> exactly that: add one new keyword "__known_attribute__" (invent a better
> name, maybe :) ), semantics exactly as with __attribute__ (including using
> the same underlying lists in our data structures), with only one single
> deviation: instead of the warning you give an error for unhandled
> attributes. Done.
Assuming you also want the better-defined standard rules about how [[]]
attributes appertain to particular entities, rather than the different
__attribute__ rules, that would suggest something like [[!some::attr]] for
the case of attributes that can't be ignored but otherwise are handled
like standard [[]] attributes.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-07-21 23:25 ` Joseph Myers
@ 2023-08-16 10:36 ` Richard Sandiford
2023-08-16 13:22 ` Joseph Myers
0 siblings, 1 reply; 19+ messages in thread
From: Richard Sandiford @ 2023-08-16 10:36 UTC (permalink / raw)
To: Joseph Myers
Cc: Michael Matz, Richard Biener, gcc-patches, polacek, jason, nathan
Joseph Myers <joseph@codesourcery.com> writes:
> On Mon, 17 Jul 2023, Michael Matz via Gcc-patches wrote:
>
>> So, essentially you want unignorable attributes, right? Then implement
>> exactly that: add one new keyword "__known_attribute__" (invent a better
>> name, maybe :) ), semantics exactly as with __attribute__ (including using
>> the same underlying lists in our data structures), with only one single
>> deviation: instead of the warning you give an error for unhandled
>> attributes. Done.
>
> Assuming you also want the better-defined standard rules about how [[]]
> attributes appertain to particular entities, rather than the different
> __attribute__ rules, that would suggest something like [[!some::attr]] for
> the case of attributes that can't be ignored but otherwise are handled
> like standard [[]] attributes.
Yeah, that would work. But I'd rather not gate the SME work on getting
an extension like that into C and C++.
As it stands, some clang maintainers pushed back against the use of
attributes for important semantics, and preferred keywords instead.
It's clear from this threads that the GCC maintainers prefer attributes
to keywords. (And it turns out that some other clang maintainers do too,
though not as strongly.)
So I think the easiest way of keeping both constituencies happy(-ish)
is to provide both standard attributes and "keywords", but allow
the "keywords" to be macros that expand to standard attributes.
Would it be OK to add support for:
[[__extension__ ...]]
to suppress the pedwarn about using [[]] prior to C2X? Then we can
predefine __arm_streaming to [[__extension__ arm::streaming]], etc.
Thanks,
Richard
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [WIP RFC] Add support for keyword-based attributes
2023-08-16 10:36 ` Richard Sandiford
@ 2023-08-16 13:22 ` Joseph Myers
2023-08-17 11:24 ` [PATCH] c: Add support for [[__extension__ ...]] Richard Sandiford
0 siblings, 1 reply; 19+ messages in thread
From: Joseph Myers @ 2023-08-16 13:22 UTC (permalink / raw)
To: Richard Sandiford
Cc: Michael Matz, Richard Biener, gcc-patches, polacek, jason, nathan
On Wed, 16 Aug 2023, Richard Sandiford via Gcc-patches wrote:
> Would it be OK to add support for:
>
> [[__extension__ ...]]
>
> to suppress the pedwarn about using [[]] prior to C2X? Then we can
That seems like a plausible feature to add.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] c: Add support for [[__extension__ ...]]
2023-08-16 13:22 ` Joseph Myers
@ 2023-08-17 11:24 ` Richard Sandiford
2023-08-17 17:07 ` Richard Biener
2023-08-18 9:51 ` Richard Sandiford
0 siblings, 2 replies; 19+ messages in thread
From: Richard Sandiford @ 2023-08-17 11:24 UTC (permalink / raw)
To: Joseph Myers; +Cc: gcc-patches
Joseph Myers <joseph@codesourcery.com> writes:
> On Wed, 16 Aug 2023, Richard Sandiford via Gcc-patches wrote:
>
>> Would it be OK to add support for:
>>
>> [[__extension__ ...]]
>>
>> to suppress the pedwarn about using [[]] prior to C2X? Then we can
>
> That seems like a plausible feature to add.
Thanks. Of course, once I actually tried it, I hit a snag:
:: isn't a single lexing token prior to C2X, and so something like:
[[__extension__ arm::streaming]]
would not be interpreted as a scoped attribute in C11. The patch
gets around that by allowing two colons in place of :: when
__extension__ is used. I realise that's pushing the bounds of
acceptability though...
I wondered about trying to require the two colons to be immediately
adjacent. But:
(a) There didn't appear to be an existing API to check that, which seemed
like a red flag. The closest I could find was get_source_text_between.
Similarly to that, it would in principle be possible to compare
two expanded locations. But...
(b) I had a vague impression that locations were allowed to drop column
information for very large inputs (maybe I'm wrong).
(c) It wouldn't cope with token pasting.
So in the end I just used a simple two-token test, like for [[ and ]].
Bootstrapped & regression-tested on aarch64-linux-gnu.
Richard
----
[[]] attributes are a recent addition to C, but as a GNU extension,
GCC allows them to be used in C11 and earlier. Normally this use
would trigger a pedwarn (for -pedantic, -Wc11-c2x-compat, etc.).
This patch allows the pedwarn to be suppressed by starting the
attribute-list with __extension__.
Also, :: is not a single lexing token prior to C2X, so it wasn't
possible to use scoped attributes in C11, even as a GNU extension.
The patch allows two colons to be used in place of :: when
__extension__ is used. No attempt is made to check whether the
two colons are immediately adjacent.
gcc/
* doc/extend.texi: Document the C [[__extension__ ...]] construct.
gcc/c/
* c-parser.cc (c_parser_std_attribute): Conditionally allow
two colons to be used in place of ::.
(c_parser_std_attribute_list): New function, split out from...
(c_parser_std_attribute_specifier): ...here. Allow the attribute-list
to start with __extension__. When it does, also allow two colons
to be used in place of ::.
gcc/testsuite/
* gcc.dg/c2x-attr-syntax-6.c: New test.
* gcc.dg/c2x-attr-syntax-7.c: Likewise.
---
gcc/c/c-parser.cc | 68 ++++++++++++++++++------
gcc/doc/extend.texi | 27 ++++++++--
gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c | 50 +++++++++++++++++
gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c | 48 +++++++++++++++++
4 files changed, 173 insertions(+), 20 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c
diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 33fe7b115ff..82e56b28446 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -5390,10 +5390,18 @@ c_parser_balanced_token_sequence (c_parser *parser)
( balanced-token-sequence[opt] )
Keywords are accepted as identifiers for this purpose.
-*/
+
+ As an extension, we permit an attribute-specifier to be:
+
+ [ [ __extension__ attribute-list ] ]
+
+ Two colons are then accepted as a synonym for ::. No attempt is made
+ to check whether the colons are immediately adjacent. LOOSE_SCOPE_P
+ indicates whether this relaxation is in effect. */
static tree
-c_parser_std_attribute (c_parser *parser, bool for_tm)
+c_parser_std_attribute (c_parser *parser, bool for_tm,
+ bool loose_scope_p = false)
{
c_token *token = c_parser_peek_token (parser);
tree ns, name, attribute;
@@ -5406,9 +5414,18 @@ c_parser_std_attribute (c_parser *parser, bool for_tm)
}
name = canonicalize_attr_name (token->value);
c_parser_consume_token (parser);
- if (c_parser_next_token_is (parser, CPP_SCOPE))
+ if (c_parser_next_token_is (parser, CPP_SCOPE)
+ || (loose_scope_p
+ && c_parser_next_token_is (parser, CPP_COLON)
+ && c_parser_peek_token (parser)->type == CPP_COLON))
{
ns = name;
+ if (c_parser_next_token_is (parser, CPP_COLON))
+ {
+ c_parser_consume_token (parser);
+ if (!c_parser_next_token_is (parser, CPP_COLON))
+ gcc_unreachable ();
+ }
c_parser_consume_token (parser);
token = c_parser_peek_token (parser);
if (token->type != CPP_NAME && token->type != CPP_KEYWORD)
@@ -5481,19 +5498,9 @@ c_parser_std_attribute (c_parser *parser, bool for_tm)
}
static tree
-c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
+c_parser_std_attribute_list (c_parser *parser, bool for_tm,
+ bool loose_scope_p = false)
{
- location_t loc = c_parser_peek_token (parser)->location;
- if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
- return NULL_TREE;
- if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
- {
- c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
- return NULL_TREE;
- }
- if (!for_tm)
- pedwarn_c11 (loc, OPT_Wpedantic,
- "ISO C does not support %<[[]]%> attributes before C2X");
tree attributes = NULL_TREE;
while (true)
{
@@ -5505,7 +5512,7 @@ c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
c_parser_consume_token (parser);
continue;
}
- tree attribute = c_parser_std_attribute (parser, for_tm);
+ tree attribute = c_parser_std_attribute (parser, for_tm, loose_scope_p);
if (attribute != error_mark_node)
{
TREE_CHAIN (attribute) = attributes;
@@ -5514,6 +5521,35 @@ c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
if (c_parser_next_token_is_not (parser, CPP_COMMA))
break;
}
+ return attributes;
+}
+
+static tree
+c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
+{
+ location_t loc = c_parser_peek_token (parser)->location;
+ if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
+ return NULL_TREE;
+ if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
+ {
+ c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
+ return NULL_TREE;
+ }
+ tree attributes;
+ if (c_parser_next_token_is_keyword (parser, RID_EXTENSION))
+ {
+ auto ext = disable_extension_diagnostics ();
+ c_parser_consume_token (parser);
+ attributes = c_parser_std_attribute_list (parser, for_tm, true);
+ restore_extension_diagnostics (ext);
+ }
+ else
+ {
+ if (!for_tm)
+ pedwarn_c11 (loc, OPT_Wpedantic,
+ "ISO C does not support %<[[]]%> attributes before C2X");
+ attributes = c_parser_std_attribute_list (parser, for_tm);
+ }
c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
return nreverse (attributes);
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 73a997276cb..d764e44cbce 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -11914,10 +11914,29 @@ macros to replace them with the customary keywords. It looks like this:
@findex __extension__
@opindex pedantic
@option{-pedantic} and other options cause warnings for many GNU C extensions.
-You can
-prevent such warnings within one expression by writing
-@code{__extension__} before the expression. @code{__extension__} has no
-effect aside from this.
+You can suppress such warnings using the keyword @code{__extension__}.
+Specifically:
+
+@itemize @bullet
+@item
+Writing @code{__extension__} before an expression prevents warnings
+about extensions within that expression.
+
+@item
+In C, writing:
+
+@smallexample
+[[__extension__ @dots{}]]
+@end smallexample
+
+suppresses warnings about using @samp{[[]]} attributes in C versions
+that predate C2X@. Since the scope token @samp{::} is not a single
+lexing token in earlier versions of C, this construct also allows two colons
+to be used in place of @code{::}. GCC does not check whether the two
+colons are immediately adjacent.
+@end itemize
+
+@code{__extension__} has no effect aside from this.
@node Incomplete Enums
@section Incomplete @code{enum} Types
diff --git a/gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c b/gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c
new file mode 100644
index 00000000000..01eff1f32df
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c
@@ -0,0 +1,50 @@
+/* Test C2x attribute syntax: use of __extension__ in C11 mode. */
+/* { dg-do compile } */
+/* { dg-options "-std=c11 -pedantic-errors" } */
+
+#define FOO ::
+#define BAR :
+
+typedef int [[__extension__ gnu :: vector_size (4)]] g1;
+typedef int [[__extension__ gnu : : vector_size (4)]] g2;
+typedef int [[__extension__ gnu FOO vector_size (4)]] g3;
+typedef int [[__extension__ gnu BAR BAR vector_size (4)]] g4;
+typedef int [[__extension__ gnu :: vector_size (sizeof (void (*)(...)))]] g5;
+typedef int [[__extension__]] g6;
+typedef int [[__extension__,]] g7;
+typedef int [[__extension__, ,,,, ,, ,]] g8;
+[[__extension__ deprecated]] int g9 ();
+[[__extension__ nodiscard]] int g10 ();
+[[__extension__ noreturn]] void g11 ();
+
+int
+cases (int x)
+{
+ switch (x)
+ {
+ case 1:
+ case 2:
+ case 4:
+ x += 1;
+ [[__extension__ fallthrough]];
+ case 19:
+ case 33:
+ x *= 2;
+ [[fallthrough]]; /* { dg-error {attributes before C2X} } */
+ case 99:
+ return x;
+
+ default:
+ return 0;
+ }
+}
+
+typedef int [[__extension__ vector_size (4)]] b1; /* { dg-error {'vector_size' attribute ignored} } */
+typedef int [[__extension__ __extension__]] b2; /* { dg-error {'extension' attribute ignored} } */
+typedef int [[__extension__ unknown_attribute]] b3; /* { dg-error {'unknown_attribute' attribute ignored} } */
+typedef int [[gnu::vector_size(4)]] b4; /* { dg-error {expected '\]' before ':'} } */
+/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
+/* { dg-error {attributes before C2X} "" { target *-*-* } .-2 } */
+typedef int [[gnu : : vector_size(4)]] b5; /* { dg-error {expected '\]' before ':'} } */
+/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
+/* { dg-error {attributes before C2X} "" { target *-*-* } .-2 } */
diff --git a/gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c b/gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c
new file mode 100644
index 00000000000..cfa9b00cfc9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c
@@ -0,0 +1,48 @@
+/* Test C2x attribute syntax: use of __extension__ in C11 mode. */
+/* { dg-do compile } */
+/* { dg-options "-std=c2x -pedantic-errors -Wc11-c2x-compat" } */
+
+#define FOO ::
+#define BAR :
+
+typedef int [[__extension__ gnu :: vector_size (4)]] g1;
+typedef int [[__extension__ gnu : : vector_size (4)]] g2;
+typedef int [[__extension__ gnu FOO vector_size (4)]] g3;
+typedef int [[__extension__ gnu BAR BAR vector_size (4)]] g4;
+typedef int [[__extension__ gnu :: vector_size (sizeof (void (*)(...)))]] g5;
+typedef int [[__extension__]] g6;
+typedef int [[__extension__,]] g7;
+typedef int [[__extension__, ,,,, ,, ,]] g8;
+[[__extension__ deprecated]] int g9 ();
+[[__extension__ nodiscard]] int g10 ();
+[[__extension__ noreturn]] void g11 ();
+
+int
+cases (int x)
+{
+ switch (x)
+ {
+ case 1:
+ case 2:
+ case 4:
+ x += 1;
+ [[__extension__ fallthrough]];
+ case 19:
+ case 33:
+ x *= 2;
+ [[fallthrough]]; /* { dg-warning {attributes before C2X} } */
+ case 99:
+ return x;
+
+ default:
+ return 0;
+ }
+}
+
+typedef int [[__extension__ vector_size (4)]] b1; /* { dg-error {'vector_size' attribute ignored} } */
+typedef int [[__extension__ __extension__]] b2; /* { dg-error {'extension' attribute ignored} } */
+typedef int [[__extension__ unknown_attribute]] b3; /* { dg-error {'unknown_attribute' attribute ignored} } */
+typedef int [[gnu::vector_size(4)]] b4; /* { dg-warning {attributes before C2X} } */
+typedef int [[gnu : : vector_size(4)]] b5; /* { dg-error {expected '\]' before ':'} } */
+/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
+/* { dg-warning {attributes before C2X} "" { target *-*-* } .-2 } */
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] c: Add support for [[__extension__ ...]]
2023-08-17 11:24 ` [PATCH] c: Add support for [[__extension__ ...]] Richard Sandiford
@ 2023-08-17 17:07 ` Richard Biener
2023-08-17 18:36 ` Richard Sandiford
2023-08-18 9:51 ` Richard Sandiford
1 sibling, 1 reply; 19+ messages in thread
From: Richard Biener @ 2023-08-17 17:07 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Joseph Myers, gcc-patches
> Am 17.08.2023 um 13:25 schrieb Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org>:
>
> Joseph Myers <joseph@codesourcery.com> writes:
>>> On Wed, 16 Aug 2023, Richard Sandiford via Gcc-patches wrote:
>>>
>>> Would it be OK to add support for:
>>>
>>> [[__extension__ ...]]
>>>
>>> to suppress the pedwarn about using [[]] prior to C2X? Then we can
>>
>> That seems like a plausible feature to add.
>
> Thanks. Of course, once I actually tried it, I hit a snag:
> :: isn't a single lexing token prior to C2X, and so something like:
>
> [[__extension__ arm::streaming]]
>
> would not be interpreted as a scoped attribute in C11. The patch
> gets around that by allowing two colons in place of :: when
> __extension__ is used. I realise that's pushing the bounds of
> acceptability though...
>
> I wondered about trying to require the two colons to be immediately
> adjacent. But:
>
> (a) There didn't appear to be an existing API to check that, which seemed
> like a red flag. The closest I could find was get_source_text_between.
IStR a cop Toben has ->prev_white or so
> Similarly to that, it would in principle be possible to compare
> two expanded locations. But...
>
> (b) I had a vague impression that locations were allowed to drop column
> information for very large inputs (maybe I'm wrong).
>
> (c) It wouldn't cope with token pasting.
>
> So in the end I just used a simple two-token test, like for [[ and ]].
> Bootstrapped & regression-tested on aarch64-linux-gnu.
>
> Richard
>
> ----
>
> [[]] attributes are a recent addition to C, but as a GNU extension,
> GCC allows them to be used in C11 and earlier. Normally this use
> would trigger a pedwarn (for -pedantic, -Wc11-c2x-compat, etc.).
>
> This patch allows the pedwarn to be suppressed by starting the
> attribute-list with __extension__.
>
> Also, :: is not a single lexing token prior to C2X, so it wasn't
> possible to use scoped attributes in C11, even as a GNU extension.
> The patch allows two colons to be used in place of :: when
> __extension__ is used. No attempt is made to check whether the
> two colons are immediately adjacent.
>
> gcc/
> * doc/extend.texi: Document the C [[__extension__ ...]] construct.
>
> gcc/c/
> * c-parser.cc (c_parser_std_attribute): Conditionally allow
> two colons to be used in place of ::.
> (c_parser_std_attribute_list): New function, split out from...
> (c_parser_std_attribute_specifier): ...here. Allow the attribute-list
> to start with __extension__. When it does, also allow two colons
> to be used in place of ::.
>
> gcc/testsuite/
> * gcc.dg/c2x-attr-syntax-6.c: New test.
> * gcc.dg/c2x-attr-syntax-7.c: Likewise.
> ---
> gcc/c/c-parser.cc | 68 ++++++++++++++++++------
> gcc/doc/extend.texi | 27 ++++++++--
> gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c | 50 +++++++++++++++++
> gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c | 48 +++++++++++++++++
> 4 files changed, 173 insertions(+), 20 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c
> create mode 100644 gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c
>
> diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
> index 33fe7b115ff..82e56b28446 100644
> --- a/gcc/c/c-parser.cc
> +++ b/gcc/c/c-parser.cc
> @@ -5390,10 +5390,18 @@ c_parser_balanced_token_sequence (c_parser *parser)
> ( balanced-token-sequence[opt] )
>
> Keywords are accepted as identifiers for this purpose.
> -*/
> +
> + As an extension, we permit an attribute-specifier to be:
> +
> + [ [ __extension__ attribute-list ] ]
> +
> + Two colons are then accepted as a synonym for ::. No attempt is made
> + to check whether the colons are immediately adjacent. LOOSE_SCOPE_P
> + indicates whether this relaxation is in effect. */
>
> static tree
> -c_parser_std_attribute (c_parser *parser, bool for_tm)
> +c_parser_std_attribute (c_parser *parser, bool for_tm,
> + bool loose_scope_p = false)
> {
> c_token *token = c_parser_peek_token (parser);
> tree ns, name, attribute;
> @@ -5406,9 +5414,18 @@ c_parser_std_attribute (c_parser *parser, bool for_tm)
> }
> name = canonicalize_attr_name (token->value);
> c_parser_consume_token (parser);
> - if (c_parser_next_token_is (parser, CPP_SCOPE))
> + if (c_parser_next_token_is (parser, CPP_SCOPE)
> + || (loose_scope_p
> + && c_parser_next_token_is (parser, CPP_COLON)
> + && c_parser_peek_token (parser)->type == CPP_COLON))
> {
> ns = name;
> + if (c_parser_next_token_is (parser, CPP_COLON))
> + {
> + c_parser_consume_token (parser);
> + if (!c_parser_next_token_is (parser, CPP_COLON))
> + gcc_unreachable ();
> + }
> c_parser_consume_token (parser);
> token = c_parser_peek_token (parser);
> if (token->type != CPP_NAME && token->type != CPP_KEYWORD)
> @@ -5481,19 +5498,9 @@ c_parser_std_attribute (c_parser *parser, bool for_tm)
> }
>
> static tree
> -c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
> +c_parser_std_attribute_list (c_parser *parser, bool for_tm,
> + bool loose_scope_p = false)
> {
> - location_t loc = c_parser_peek_token (parser)->location;
> - if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
> - return NULL_TREE;
> - if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
> - {
> - c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
> - return NULL_TREE;
> - }
> - if (!for_tm)
> - pedwarn_c11 (loc, OPT_Wpedantic,
> - "ISO C does not support %<[[]]%> attributes before C2X");
> tree attributes = NULL_TREE;
> while (true)
> {
> @@ -5505,7 +5512,7 @@ c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
> c_parser_consume_token (parser);
> continue;
> }
> - tree attribute = c_parser_std_attribute (parser, for_tm);
> + tree attribute = c_parser_std_attribute (parser, for_tm, loose_scope_p);
> if (attribute != error_mark_node)
> {
> TREE_CHAIN (attribute) = attributes;
> @@ -5514,6 +5521,35 @@ c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
> if (c_parser_next_token_is_not (parser, CPP_COMMA))
> break;
> }
> + return attributes;
> +}
> +
> +static tree
> +c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
> +{
> + location_t loc = c_parser_peek_token (parser)->location;
> + if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
> + return NULL_TREE;
> + if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
> + {
> + c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
> + return NULL_TREE;
> + }
> + tree attributes;
> + if (c_parser_next_token_is_keyword (parser, RID_EXTENSION))
> + {
> + auto ext = disable_extension_diagnostics ();
> + c_parser_consume_token (parser);
> + attributes = c_parser_std_attribute_list (parser, for_tm, true);
> + restore_extension_diagnostics (ext);
> + }
> + else
> + {
> + if (!for_tm)
> + pedwarn_c11 (loc, OPT_Wpedantic,
> + "ISO C does not support %<[[]]%> attributes before C2X");
> + attributes = c_parser_std_attribute_list (parser, for_tm);
> + }
> c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
> c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
> return nreverse (attributes);
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 73a997276cb..d764e44cbce 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -11914,10 +11914,29 @@ macros to replace them with the customary keywords. It looks like this:
> @findex __extension__
> @opindex pedantic
> @option{-pedantic} and other options cause warnings for many GNU C extensions.
> -You can
> -prevent such warnings within one expression by writing
> -@code{__extension__} before the expression. @code{__extension__} has no
> -effect aside from this.
> +You can suppress such warnings using the keyword @code{__extension__}.
> +Specifically:
> +
> +@itemize @bullet
> +@item
> +Writing @code{__extension__} before an expression prevents warnings
> +about extensions within that expression.
> +
> +@item
> +In C, writing:
> +
> +@smallexample
> +[[__extension__ @dots{}]]
> +@end smallexample
> +
> +suppresses warnings about using @samp{[[]]} attributes in C versions
> +that predate C2X@. Since the scope token @samp{::} is not a single
> +lexing token in earlier versions of C, this construct also allows two colons
> +to be used in place of @code{::}. GCC does not check whether the two
> +colons are immediately adjacent.
> +@end itemize
> +
> +@code{__extension__} has no effect aside from this.
>
> @node Incomplete Enums
> @section Incomplete @code{enum} Types
> diff --git a/gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c b/gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c
> new file mode 100644
> index 00000000000..01eff1f32df
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c
> @@ -0,0 +1,50 @@
> +/* Test C2x attribute syntax: use of __extension__ in C11 mode. */
> +/* { dg-do compile } */
> +/* { dg-options "-std=c11 -pedantic-errors" } */
> +
> +#define FOO ::
> +#define BAR :
> +
> +typedef int [[__extension__ gnu :: vector_size (4)]] g1;
> +typedef int [[__extension__ gnu : : vector_size (4)]] g2;
> +typedef int [[__extension__ gnu FOO vector_size (4)]] g3;
> +typedef int [[__extension__ gnu BAR BAR vector_size (4)]] g4;
> +typedef int [[__extension__ gnu :: vector_size (sizeof (void (*)(...)))]] g5;
> +typedef int [[__extension__]] g6;
> +typedef int [[__extension__,]] g7;
> +typedef int [[__extension__, ,,,, ,, ,]] g8;
> +[[__extension__ deprecated]] int g9 ();
> +[[__extension__ nodiscard]] int g10 ();
> +[[__extension__ noreturn]] void g11 ();
> +
> +int
> +cases (int x)
> +{
> + switch (x)
> + {
> + case 1:
> + case 2:
> + case 4:
> + x += 1;
> + [[__extension__ fallthrough]];
> + case 19:
> + case 33:
> + x *= 2;
> + [[fallthrough]]; /* { dg-error {attributes before C2X} } */
> + case 99:
> + return x;
> +
> + default:
> + return 0;
> + }
> +}
> +
> +typedef int [[__extension__ vector_size (4)]] b1; /* { dg-error {'vector_size' attribute ignored} } */
> +typedef int [[__extension__ __extension__]] b2; /* { dg-error {'extension' attribute ignored} } */
> +typedef int [[__extension__ unknown_attribute]] b3; /* { dg-error {'unknown_attribute' attribute ignored} } */
> +typedef int [[gnu::vector_size(4)]] b4; /* { dg-error {expected '\]' before ':'} } */
> +/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
> +/* { dg-error {attributes before C2X} "" { target *-*-* } .-2 } */
> +typedef int [[gnu : : vector_size(4)]] b5; /* { dg-error {expected '\]' before ':'} } */
> +/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
> +/* { dg-error {attributes before C2X} "" { target *-*-* } .-2 } */
> diff --git a/gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c b/gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c
> new file mode 100644
> index 00000000000..cfa9b00cfc9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c
> @@ -0,0 +1,48 @@
> +/* Test C2x attribute syntax: use of __extension__ in C11 mode. */
> +/* { dg-do compile } */
> +/* { dg-options "-std=c2x -pedantic-errors -Wc11-c2x-compat" } */
> +
> +#define FOO ::
> +#define BAR :
> +
> +typedef int [[__extension__ gnu :: vector_size (4)]] g1;
> +typedef int [[__extension__ gnu : : vector_size (4)]] g2;
> +typedef int [[__extension__ gnu FOO vector_size (4)]] g3;
> +typedef int [[__extension__ gnu BAR BAR vector_size (4)]] g4;
> +typedef int [[__extension__ gnu :: vector_size (sizeof (void (*)(...)))]] g5;
> +typedef int [[__extension__]] g6;
> +typedef int [[__extension__,]] g7;
> +typedef int [[__extension__, ,,,, ,, ,]] g8;
> +[[__extension__ deprecated]] int g9 ();
> +[[__extension__ nodiscard]] int g10 ();
> +[[__extension__ noreturn]] void g11 ();
> +
> +int
> +cases (int x)
> +{
> + switch (x)
> + {
> + case 1:
> + case 2:
> + case 4:
> + x += 1;
> + [[__extension__ fallthrough]];
> + case 19:
> + case 33:
> + x *= 2;
> + [[fallthrough]]; /* { dg-warning {attributes before C2X} } */
> + case 99:
> + return x;
> +
> + default:
> + return 0;
> + }
> +}
> +
> +typedef int [[__extension__ vector_size (4)]] b1; /* { dg-error {'vector_size' attribute ignored} } */
> +typedef int [[__extension__ __extension__]] b2; /* { dg-error {'extension' attribute ignored} } */
> +typedef int [[__extension__ unknown_attribute]] b3; /* { dg-error {'unknown_attribute' attribute ignored} } */
> +typedef int [[gnu::vector_size(4)]] b4; /* { dg-warning {attributes before C2X} } */
> +typedef int [[gnu : : vector_size(4)]] b5; /* { dg-error {expected '\]' before ':'} } */
> +/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
> +/* { dg-warning {attributes before C2X} "" { target *-*-* } .-2 } */
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] c: Add support for [[__extension__ ...]]
2023-08-17 17:07 ` Richard Biener
@ 2023-08-17 18:36 ` Richard Sandiford
0 siblings, 0 replies; 19+ messages in thread
From: Richard Sandiford @ 2023-08-17 18:36 UTC (permalink / raw)
To: Richard Biener; +Cc: Joseph Myers, gcc-patches
Richard Biener <richard.guenther@gmail.com> writes:
>> Am 17.08.2023 um 13:25 schrieb Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org>:
>>
>> Joseph Myers <joseph@codesourcery.com> writes:
>>>> On Wed, 16 Aug 2023, Richard Sandiford via Gcc-patches wrote:
>>>>
>>>> Would it be OK to add support for:
>>>>
>>>> [[__extension__ ...]]
>>>>
>>>> to suppress the pedwarn about using [[]] prior to C2X? Then we can
>>>
>>> That seems like a plausible feature to add.
>>
>> Thanks. Of course, once I actually tried it, I hit a snag:
>> :: isn't a single lexing token prior to C2X, and so something like:
>>
>> [[__extension__ arm::streaming]]
>>
>> would not be interpreted as a scoped attribute in C11. The patch
>> gets around that by allowing two colons in place of :: when
>> __extension__ is used. I realise that's pushing the bounds of
>> acceptability though...
>>
>> I wondered about trying to require the two colons to be immediately
>> adjacent. But:
>>
>> (a) There didn't appear to be an existing API to check that, which seemed
>> like a red flag. The closest I could find was get_source_text_between.
>
> IStR a cop Toben has ->prev_white or so
Ah, thanks.
if (c_parser_next_token_is (parser, CPP_SCOPE)
|| (loose_scope_p
&& c_parser_next_token_is (parser, CPP_COLON)
&& c_parser_peek_2nd_token (parser)->type == CPP_COLON
&& !(c_parser_peek_2nd_token (parser)->flags & PREV_WHITE)))
seems to work for (i.e. reject):
typedef int [[__extension__ gnu : : vector_size (4)]] g3;
typedef int [[__extension__ gnu :/**/: vector_size (4)]] g13;
but not:
#define BAR :
typedef int [[__extension__ gnu BAR BAR vector_size (4)]] g5;
#define JOIN(A, B) A/**/B
typedef int [[__extension__ gnu JOIN(:,:) vector_size (4)]] g14;
I now realise the patch was peeking at the wrong token. Will fix,
and add more tests.
Richard
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] c: Add support for [[__extension__ ...]]
2023-08-17 11:24 ` [PATCH] c: Add support for [[__extension__ ...]] Richard Sandiford
2023-08-17 17:07 ` Richard Biener
@ 2023-08-18 9:51 ` Richard Sandiford
2023-08-18 19:51 ` Joseph Myers
1 sibling, 1 reply; 19+ messages in thread
From: Richard Sandiford @ 2023-08-18 9:51 UTC (permalink / raw)
To: Joseph Myers; +Cc: gcc-patches
Richard Sandiford <richard.sandiford@arm.com> writes:
> Joseph Myers <joseph@codesourcery.com> writes:
>> On Wed, 16 Aug 2023, Richard Sandiford via Gcc-patches wrote:
>>
>>> Would it be OK to add support for:
>>>
>>> [[__extension__ ...]]
>>>
>>> to suppress the pedwarn about using [[]] prior to C2X? Then we can
>>
>> That seems like a plausible feature to add.
>
> Thanks. Of course, once I actually tried it, I hit a snag:
> :: isn't a single lexing token prior to C2X, and so something like:
>
> [[__extension__ arm::streaming]]
>
> would not be interpreted as a scoped attribute in C11. The patch
> gets around that by allowing two colons in place of :: when
> __extension__ is used. I realise that's pushing the bounds of
> acceptability though...
>
> I wondered about trying to require the two colons to be immediately
> adjacent. But:
>
> (a) There didn't appear to be an existing API to check that, which seemed
> like a red flag. The closest I could find was get_source_text_between.
>
> Similarly to that, it would in principle be possible to compare
> two expanded locations. But...
>
> (b) I had a vague impression that locations were allowed to drop column
> information for very large inputs (maybe I'm wrong).
>
> (c) It wouldn't cope with token pasting.
>
> So in the end I just used a simple two-token test, like for [[ and ]].
>
> Bootstrapped & regression-tested on aarch64-linux-gnu.
Gah, as mentioned yesterday, the patch was peeking the wrong token.
I've fixed that, and added corresponding tests. Sorry for missing
it first time.
Richard
-----
[[]] attributes are a recent addition to C, but as a GNU extension,
GCC allows them to be used in C11 and earlier. Normally this use
would trigger a pedwarn (for -pedantic, -Wc11-c2x-compat, etc.).
This patch allows the pedwarn to be suppressed by starting the
attribute-list with __extension__.
Also, :: is not a single lexing token prior to C2X, so it wasn't
possible to use scoped attributes in C11, even as a GNU extension.
The patch allows two colons to be used in place of :: when
__extension__ is used. No attempt is made to check whether the
two colons are immediately adjacent.
gcc/
* doc/extend.texi: Document the C [[__extension__ ...]] construct.
gcc/c/
* c-parser.cc (c_parser_std_attribute): Conditionally allow
two colons to be used in place of ::.
(c_parser_std_attribute_list): New function, split out from...
(c_parser_std_attribute_specifier): ...here. Allow the attribute-list
to start with __extension__. When it does, also allow two colons
to be used in place of ::.
gcc/testsuite/
* gcc.dg/c2x-attr-syntax-6.c: New test.
* gcc.dg/c2x-attr-syntax-7.c: Likewise.
---
gcc/c/c-parser.cc | 64 ++++++++++++++++++------
gcc/doc/extend.texi | 27 ++++++++--
gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c | 62 +++++++++++++++++++++++
gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c | 60 ++++++++++++++++++++++
4 files changed, 193 insertions(+), 20 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c
create mode 100644 gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c
diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 33fe7b115ff..ca60c51ddb2 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -5390,10 +5390,18 @@ c_parser_balanced_token_sequence (c_parser *parser)
( balanced-token-sequence[opt] )
Keywords are accepted as identifiers for this purpose.
-*/
+
+ As an extension, we permit an attribute-specifier to be:
+
+ [ [ __extension__ attribute-list ] ]
+
+ Two colons are then accepted as a synonym for ::. No attempt is made
+ to check whether the colons are immediately adjacent. LOOSE_SCOPE_P
+ indicates whether this relaxation is in effect. */
static tree
-c_parser_std_attribute (c_parser *parser, bool for_tm)
+c_parser_std_attribute (c_parser *parser, bool for_tm,
+ bool loose_scope_p = false)
{
c_token *token = c_parser_peek_token (parser);
tree ns, name, attribute;
@@ -5406,9 +5414,14 @@ c_parser_std_attribute (c_parser *parser, bool for_tm)
}
name = canonicalize_attr_name (token->value);
c_parser_consume_token (parser);
- if (c_parser_next_token_is (parser, CPP_SCOPE))
+ if (c_parser_next_token_is (parser, CPP_SCOPE)
+ || (loose_scope_p
+ && c_parser_next_token_is (parser, CPP_COLON)
+ && c_parser_peek_2nd_token (parser)->type == CPP_COLON))
{
ns = name;
+ if (c_parser_next_token_is (parser, CPP_COLON))
+ c_parser_consume_token (parser);
c_parser_consume_token (parser);
token = c_parser_peek_token (parser);
if (token->type != CPP_NAME && token->type != CPP_KEYWORD)
@@ -5481,19 +5494,9 @@ c_parser_std_attribute (c_parser *parser, bool for_tm)
}
static tree
-c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
+c_parser_std_attribute_list (c_parser *parser, bool for_tm,
+ bool loose_scope_p = false)
{
- location_t loc = c_parser_peek_token (parser)->location;
- if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
- return NULL_TREE;
- if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
- {
- c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
- return NULL_TREE;
- }
- if (!for_tm)
- pedwarn_c11 (loc, OPT_Wpedantic,
- "ISO C does not support %<[[]]%> attributes before C2X");
tree attributes = NULL_TREE;
while (true)
{
@@ -5505,7 +5508,7 @@ c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
c_parser_consume_token (parser);
continue;
}
- tree attribute = c_parser_std_attribute (parser, for_tm);
+ tree attribute = c_parser_std_attribute (parser, for_tm, loose_scope_p);
if (attribute != error_mark_node)
{
TREE_CHAIN (attribute) = attributes;
@@ -5514,6 +5517,35 @@ c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
if (c_parser_next_token_is_not (parser, CPP_COMMA))
break;
}
+ return attributes;
+}
+
+static tree
+c_parser_std_attribute_specifier (c_parser *parser, bool for_tm)
+{
+ location_t loc = c_parser_peek_token (parser)->location;
+ if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
+ return NULL_TREE;
+ if (!c_parser_require (parser, CPP_OPEN_SQUARE, "expected %<[%>"))
+ {
+ c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
+ return NULL_TREE;
+ }
+ tree attributes;
+ if (c_parser_next_token_is_keyword (parser, RID_EXTENSION))
+ {
+ auto ext = disable_extension_diagnostics ();
+ c_parser_consume_token (parser);
+ attributes = c_parser_std_attribute_list (parser, for_tm, true);
+ restore_extension_diagnostics (ext);
+ }
+ else
+ {
+ if (!for_tm)
+ pedwarn_c11 (loc, OPT_Wpedantic,
+ "ISO C does not support %<[[]]%> attributes before C2X");
+ attributes = c_parser_std_attribute_list (parser, for_tm);
+ }
c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, "expected %<]%>");
return nreverse (attributes);
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index f657032cbef..d9a907cb1cb 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -11938,10 +11938,29 @@ macros to replace them with the customary keywords. It looks like this:
@findex __extension__
@opindex pedantic
@option{-pedantic} and other options cause warnings for many GNU C extensions.
-You can
-prevent such warnings within one expression by writing
-@code{__extension__} before the expression. @code{__extension__} has no
-effect aside from this.
+You can suppress such warnings using the keyword @code{__extension__}.
+Specifically:
+
+@itemize @bullet
+@item
+Writing @code{__extension__} before an expression prevents warnings
+about extensions within that expression.
+
+@item
+In C, writing:
+
+@smallexample
+[[__extension__ @dots{}]]
+@end smallexample
+
+suppresses warnings about using @samp{[[]]} attributes in C versions
+that predate C2X@. Since the scope token @samp{::} is not a single
+lexing token in earlier versions of C, this construct also allows two colons
+to be used in place of @code{::}. GCC does not check whether the two
+colons are immediately adjacent.
+@end itemize
+
+@code{__extension__} has no effect aside from this.
@node Incomplete Enums
@section Incomplete @code{enum} Types
diff --git a/gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c b/gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c
new file mode 100644
index 00000000000..9e5f65ce469
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/c2x-attr-syntax-6.c
@@ -0,0 +1,62 @@
+/* Test C2x attribute syntax: use of __extension__ in C11 mode. */
+/* { dg-do compile } */
+/* { dg-options "-std=c11 -pedantic-errors" } */
+
+#define FOO ::
+#define BAR :
+#define JOIN(A, B) A/**/B
+#define JOIN2(A, B) A##B
+
+typedef int [[__extension__ gnu::vector_size (4)]] g1;
+typedef int [[__extension__ gnu :: vector_size (4)]] g2;
+typedef int [[__extension__ gnu : : vector_size (4)]] g3;
+typedef int [[__extension__ gnu: :vector_size (4)]] g4;
+typedef int [[__extension__ gnu FOO vector_size (4)]] g5;
+typedef int [[__extension__ gnu BAR BAR vector_size (4)]] g6;
+typedef int [[__extension__ gnu :/**/: vector_size (4)]] g7;
+typedef int [[__extension__ gnu JOIN(:,:) vector_size (4)]] g8;
+typedef int [[__extension__ gnu :: vector_size (sizeof (void (*)(...)))]] g10;
+typedef int [[__extension__]] g11;
+typedef int [[__extension__,]] g12;
+typedef int [[__extension__, ,,,, ,, ,]] g13;
+[[__extension__ deprecated]] int g14 ();
+[[__extension__ nodiscard]] int g15 ();
+[[__extension__ noreturn]] void g16 ();
+
+int
+cases (int x)
+{
+ switch (x)
+ {
+ case 1:
+ case 2:
+ case 4:
+ x += 1;
+ [[__extension__ fallthrough]];
+ case 19:
+ case 33:
+ x *= 2;
+ [[fallthrough]]; /* { dg-error {attributes before C2X} } */
+ case 99:
+ return x;
+
+ default:
+ return 0;
+ }
+}
+
+typedef int [[__extension__ vector_size (4)]] b1; /* { dg-error {'vector_size' attribute ignored} } */
+typedef int [[__extension__ __extension__]] b2; /* { dg-error {'extension' attribute ignored} } */
+typedef int [[__extension__ unknown_attribute]] b3; /* { dg-error {'unknown_attribute' attribute ignored} } */
+typedef int [[__extension__ gnu:vector_size(4)]] b4; /* { dg-error {expected '\]' before ':'} } */
+/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
+typedef int [[__extension__ gnu JOIN2(:,:) vector_size (4)]] b5; /* { dg-error {pasting ":" and ":" does not give a valid preprocessing token} } */
+typedef int [[gnu::vector_size(4)]] b6; /* { dg-error {expected '\]' before ':'} } */
+/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
+/* { dg-error {attributes before C2X} "" { target *-*-* } .-2 } */
+typedef int [[gnu : : vector_size(4)]] b7; /* { dg-error {expected '\]' before ':'} } */
+/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
+/* { dg-error {attributes before C2X} "" { target *-*-* } .-2 } */
+typedef int [[gnu : vector_size(4)]] b8; /* { dg-error {expected '\]' before ':'} } */
+/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
+/* { dg-error {attributes before C2X} "" { target *-*-* } .-2 } */
diff --git a/gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c b/gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c
new file mode 100644
index 00000000000..702f733b171
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/c2x-attr-syntax-7.c
@@ -0,0 +1,60 @@
+/* Test C2x attribute syntax: use of __extension__ in C11 mode. */
+/* { dg-do compile } */
+/* { dg-options "-std=c2x -pedantic-errors -Wc11-c2x-compat" } */
+
+#define FOO ::
+#define BAR :
+#define JOIN(A, B) A/**/B
+#define JOIN2(A, B) A##B
+
+typedef int [[__extension__ gnu::vector_size (4)]] g1;
+typedef int [[__extension__ gnu :: vector_size (4)]] g2;
+typedef int [[__extension__ gnu : : vector_size (4)]] g3;
+typedef int [[__extension__ gnu: :vector_size (4)]] g4;
+typedef int [[__extension__ gnu FOO vector_size (4)]] g5;
+typedef int [[__extension__ gnu BAR BAR vector_size (4)]] g6;
+typedef int [[__extension__ gnu :/**/: vector_size (4)]] g7;
+typedef int [[__extension__ gnu JOIN(:,:) vector_size (4)]] g8;
+typedef int [[__extension__ gnu :: vector_size (sizeof (void (*)(...)))]] g10;
+typedef int [[__extension__]] g11;
+typedef int [[__extension__,]] g12;
+typedef int [[__extension__, ,,,, ,, ,]] g13;
+[[__extension__ deprecated]] int g14 ();
+[[__extension__ nodiscard]] int g15 ();
+[[__extension__ noreturn]] void g16 ();
+
+int
+cases (int x)
+{
+ switch (x)
+ {
+ case 1:
+ case 2:
+ case 4:
+ x += 1;
+ [[__extension__ fallthrough]];
+ case 19:
+ case 33:
+ x *= 2;
+ [[fallthrough]]; /* { dg-warning {attributes before C2X} } */
+ case 99:
+ return x;
+
+ default:
+ return 0;
+ }
+}
+
+typedef int [[__extension__ vector_size (4)]] b1; /* { dg-error {'vector_size' attribute ignored} } */
+typedef int [[__extension__ __extension__]] b2; /* { dg-error {'extension' attribute ignored} } */
+typedef int [[__extension__ unknown_attribute]] b3; /* { dg-error {'unknown_attribute' attribute ignored} } */
+typedef int [[__extension__ gnu:vector_size(4)]] b4; /* { dg-error {expected '\]' before ':'} } */
+/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
+typedef int [[__extension__ gnu JOIN2(:,:) vector_size (4)]] b5;
+typedef int [[gnu::vector_size(4)]] b6; /* { dg-warning {attributes before C2X} } */
+typedef int [[gnu : : vector_size(4)]] b7; /* { dg-error {expected '\]' before ':'} } */
+/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
+/* { dg-warning {attributes before C2X} "" { target *-*-* } .-2 } */
+typedef int [[gnu : vector_size(4)]] b8; /* { dg-error {expected '\]' before ':'} } */
+/* { dg-error {'gnu' attribute ignored} "" { target *-*-* } .-1 } */
+/* { dg-warning {attributes before C2X} "" { target *-*-* } .-2 } */
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] c: Add support for [[__extension__ ...]]
2023-08-18 9:51 ` Richard Sandiford
@ 2023-08-18 19:51 ` Joseph Myers
0 siblings, 0 replies; 19+ messages in thread
From: Joseph Myers @ 2023-08-18 19:51 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
On Fri, 18 Aug 2023, Richard Sandiford via Gcc-patches wrote:
> [[]] attributes are a recent addition to C, but as a GNU extension,
> GCC allows them to be used in C11 and earlier. Normally this use
> would trigger a pedwarn (for -pedantic, -Wc11-c2x-compat, etc.).
>
> This patch allows the pedwarn to be suppressed by starting the
> attribute-list with __extension__.
>
> Also, :: is not a single lexing token prior to C2X, so it wasn't
> possible to use scoped attributes in C11, even as a GNU extension.
> The patch allows two colons to be used in place of :: when
> __extension__ is used. No attempt is made to check whether the
> two colons are immediately adjacent.
>
> gcc/
> * doc/extend.texi: Document the C [[__extension__ ...]] construct.
>
> gcc/c/
> * c-parser.cc (c_parser_std_attribute): Conditionally allow
> two colons to be used in place of ::.
> (c_parser_std_attribute_list): New function, split out from...
> (c_parser_std_attribute_specifier): ...here. Allow the attribute-list
> to start with __extension__. When it does, also allow two colons
> to be used in place of ::.
>
> gcc/testsuite/
> * gcc.dg/c2x-attr-syntax-6.c: New test.
> * gcc.dg/c2x-attr-syntax-7.c: Likewise.
OK.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 19+ messages in thread