Page MenuHomePhabricator

[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 Timeline

egorzhdan created this revision.Mar 9 2022, 4:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 4:46 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
egorzhdan requested review of this revision.Mar 9 2022, 4:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 4:46 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
egorzhdan updated this revision to Diff 414068.Mar 9 2022, 4:48 AM

Remove unused include

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.

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

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.

why is whitespace the correct separator as opposed to a comma-delimited list?

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.

Also, I'd expect there to be some documentation changes along with this patch and a release note.

Good point, thanks, I will add those to this patch.

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

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.

I don't yet think it's a *bad* construct to support...

why is whitespace the correct separator as opposed to a comma-delimited list?

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.

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.

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?

I think this is reasonable, and as you've mentioned, it also leaves the opportunity to extend this syntax later should the need arise.
I will adjust this patch shortly.

egorzhdan updated this revision to Diff 415199.Mar 14 2022, 1:25 PM
  • Only allow one attribute syntax style per directive
  • Adjust documentation
  • Add a release note
aaron.ballman accepted this revision.Mar 15 2022, 9:58 AM

Aside from a minor testing nit, this LGTM. Thank you for the new functionality!

clang/test/Parser/pragma-multiple-attributes.cpp
1–11 ↗(On Diff #415199)

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).

This revision is now accepted and ready to land.Mar 15 2022, 9:58 AM

Merge two test files into one to speed up testing

Thanks for the review @aaron.ballman!

  • Fix off-by-one error in fix-it generation logic: the last SubjectMatchRule

(SubjectMatchRule_variable_not_is_parameter) was not handled properly

  • Add a test for this
This revision was landed with ongoing or failed builds.Mar 18 2022, 5:20 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Mar 18 2022, 5:44 AM

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 :)

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 :)

That's strange, I didn't notice this, thanks for the heads up.