Paths
| Differential D121283
[Clang] Support multiple attributes in a single pragma ClosedPublic Authored by egorzhdan on Mar 9 2022, 4:46 AM.
Details Summary This adds support for multiple attributes in #pragma clang attribute push, for example: #pragma clang attribute push (__attribute__((disable_sanitizer_instrumentation)) __attribute__((section("S"))), apply_to=variable(is_global))or #pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function) Related attributes can now be applied with a single pragma, which makes it harder for developers to make an accidental error later when editing the code. rdar://78269653
Diff Detail
Event TimelineComment Actions Thanks for this, I plan on giving it a more thorough review when I can. But the summary led me to a design question: why do we support multiple attribute *specifiers* in the same pragma? I would not expect to be able to mix attribute styles in the same pragma given that all of the individual styles allow you to specify multiple attributes within a single specifier. e.g., https://godbolt.org/z/9v7r6Eanz __declspec(deprecated, naked) void f();
__attribute__((annotate("test"), constructor(0))) void g();
[[clang::annotate("test"), gnu::constructor(0)]] void h();What's the use case for letting someone mix and match attribute styles as in: #pragma clang attribute push ([[clang::disable_tail_calls]] __attribute__((annotate("test"))), apply_to = function)and why is whitespace the correct separator as opposed to a comma-delimited list? (Note, I'm still thinking about whether this approach poses any actual problems; it may be perfectly fine, but it'd help to know why we're being lax in mixing forms and what the rationale is for whitespace separation; that's not a common approach to lists of things in C or C++). Also, I'd expect there to be some documentation changes along with this patch and a release note. Comment Actions
I don't think I have a strong use case for this. It seemed consistent with the way multiple attributes can be added for individual declarations, e.g. __attribute__((cdecl)) __declspec(dllexport) [[noreturn]] void f(). But we can prohibit multiple specifiers within a single pragma if you think that this is not a good construct to support.
My motivation for this was also consistency with the syntax for attributes in individual declarations. Given that attribute specifiers are delimited by space for individual declarations (__attribute__((cdecl)) __attribute__((noreturn)) void f()), I think it would be unintuitive to require commas for attribute specifiers in pragmas when we could instead reuse the existing syntax of space-delimited attribute specifiers.
Good point, thanks, I will add those to this patch. Comment Actions
I don't yet think it's a *bad* construct to support...
Thanks, that makes some sense to me, but at the same time, I can't think of another pragma that behaves similarly off the top of my head (usually, lists of things have a delimiter other than whitespace), so it's kind of unintuitive either way. As a thought experiment, would it make sense to lift the restriction on the number of attributes allowed in a pragma, but not allow multiple attribute specifiers? e.g., // These are fine #pragma clang attribute push ([[clang::disable_tail_calls, noreturn]], apply_to = function) #pragma clang attribute pop #pragma clang attribute push (__attribute__((noreturn, noinline)), apply_to = function) #pragma clang attribute pop // These are not allowed #pragma clang attribute push ([[clang::disable_tail_calls]] [[noreturn]], apply_to = function) #pragma clang attribute pop #pragma clang attribute push (__attribute__((noreturn)) __attribute__((noinline)), apply_to = function) #pragma clang attribute pop #pragma clang attribute push ([[clang::disable_tail_calls]] __attribute__((noreturn)), apply_to = function) #pragma clang attribute pop This still allows you to specify more than one attribute in the pragma, but removes the concerns about how to separate the syntaxes (whitespace or another delimiter) while still leaving that design open in the future if there's a strong need to mix syntaxes. The pragma attribute feature has a lot of power to it, but it also comes with a lot of risk to users, so it's a feature that I think we should be cautious about extending (in general). I think what you propose here could very well be reasonable, but I'm a bit worried that there's not a clear need for that amount of flexibility, which is why I'm sort of leaning towards being more restrictive here. However, this isn't exposing any functionality that users can't already do the long way with multiple pragmas, so I don't see a blocking concern with the current patch either. Comment Actions
I think this is reasonable, and as you've mentioned, it also leaves the opportunity to extend this syntax later should the need arise. Comment Actions
Comment Actions Aside from a minor testing nit, this LGTM. Thank you for the new functionality!
This revision is now accepted and ready to land.Mar 15 2022, 9:58 AM Comment Actions
(SubjectMatchRule_variable_not_is_parameter) was not handled properly
This revision was landed with ongoing or failed builds.Mar 18 2022, 5:20 AM Closed by commit rG33a9eac6aaa4: [Clang] Support multiple attributes in a single pragma (authored by egorzhdan). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions The commit message got a bit mutilated: 33a9eac6aaa495fce6fd9b17cd48aa57a95461e6 Just FYI in case you didn't notice. In that case, update your commit workflow to make sure this doesn't happen next time :) Comment Actions
That's strange, I didn't notice this, thanks for the heads up.
Revision Contents
Diff 414067 clang/include/clang/Basic/AttrSubjectMatchRules.h
clang/include/clang/Basic/DiagnosticParseKinds.td
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParsePragma.cpp
clang/test/AST/pragma-multiple-attributes-declspec.cpp
clang/test/AST/pragma-multiple-attributes.cpp
clang/test/FixIt/fixit-pragma-attribute.c
clang/test/FixIt/fixit-pragma-attribute.cpp
clang/test/Parser/pragma-attribute-declspec.cpp
clang/test/Parser/pragma-attribute.cpp
clang/test/Parser/pragma-multiple-attributes.cpp
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I think this entire test file can now be folded back into pragma-attribute.cpp (this way we don't have to pay the overhead of another execution of Clang to test the functionality).