Re: [WIP RFC] Add support for keyword-based attributes

[Richard Biener <richard.guenther@gmail.com>]

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