Page MenuHomePhabricator

Add support for "labels" on push/pop directives in #pragma clang attribute
ClosedPublic

Authored by erik.pilkington on Dec 12 2018, 4:26 PM.

Details

Summary

One problem with defining macros that expand to _Pragma("clang attribute")` is that they don't nest very well:

// In some header...
#define ASSUME_X_BEGIN _Pragma ("clang attribute push(__attribute__((x)), apply_to=variable)")
#define ASSUME_X_END _Pragma ("clang attribute pop")

#define ASSUME_Y_BEGIN _Pragma("clang attribute push(__attribute__((y)), apply_to=variable)")
#define ASSUME_Y_END _Pragma("clang attribute pop")

// in some source file...

ASSUME_X_BEGIN
// lots of code
ASSUME_Y_BEGIN
// some more code
ASSUME_X_END // oops! Popped Y, but meant to pop X.
// some other code
ASSUME_Y_END // ditto

This patch adds a way to fix this by adding labels to push/pop directives. A clang attribute pops with a label will only pop a pushed group with that same label. Macro authors can then add a unique label to their BEGIN/END functions, which would cause the code above to work fine.

Thanks for taking a look!
Erik

Diff Detail

Repository
rC Clang

Event Timeline

aaron.ballman added inline comments.Dec 14 2018, 5:23 AM
clang/docs/LanguageExtensions.rst
2667 ↗(On Diff #177971)

The documentation reads like the label is optional, but the examples make it look required (because all examples have the label present). Can you clarify what happens when the label is omitted and keep an example that shows it?

clang/include/clang/Basic/DiagnosticSemaKinds.td
840–842 ↗(On Diff #177971)

Can this be combined with the diagnostic above using %select (given that the wording is basically identical)?

clang/include/clang/Sema/Sema.h
8504 ↗(On Diff #177971)

Can you sprinkle some const-correctness around to make this const IdentifierInfo *?

clang/lib/Sema/SemaAttr.cpp
648 ↗(On Diff #177971)

corrisponds -> corresponds

659 ↗(On Diff #177971)

You can pass in *Entry.Attribute here instead of calling getName() and it will properly quote the results.

clang/test/Sema/pragma-attribute-label.c
7 ↗(On Diff #177971)

Should we really treat this as an error? It seems to me that this should be a warning because pop without a label could be viewed as "I don't care what I'm popping, just pop it". Still worth warning about, but maybe not worth stopping a build over.

15 ↗(On Diff #177971)

I feel like this should be diagnosed, perhaps even as an error. The user provided labels but then got the push and pop order wrong when explicitly saying what to pop. That seems more likely to be a logic error on the user's part.

dexonsmith added inline comments.Dec 14 2018, 7:00 AM
clang/test/Sema/pragma-attribute-label.c
7 ↗(On Diff #177971)

IMO this is most likely to be an implementation error on the part of a macro author, where the END macro is missing the label used in BEGIN. This makes the macro pair unsafe to mix with other macros. If the macro author doesn’t want safety, why use a label in the BEGIN macro at all?

I see you’re envisioning this being used directly by an end-user, which I suppose is plausible, but I think the same logic applies. Why add a label to push if you don’t want to be precise about pop?

15 ↗(On Diff #177971)

On the contrary, the user is using two differently-named and independent macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END) and has no idea they are implemented with _Pragma(“clang attribute ...”) under the hood. The point is to give the same result as two independent pragma pairs, whose regions do not need to be nested.

aaron.ballman added inline comments.Dec 14 2018, 8:00 AM
clang/test/Sema/pragma-attribute-label.c
7 ↗(On Diff #177971)

Why add a label to push if you don’t want to be precise about pop?

Why is this important enough to fail everyone's build over it as opposed to warning users that they've done something that could be a bad code smell and let -Werror usage decide whether to fail the build or not? It seems like an extreme measure for something that has explicable "fallback" behavior.

15 ↗(On Diff #177971)

On the contrary, the user is using two differently-named and independent macro pairs (A_BEGIN/A_END vs B_BEGIN/B_END)

I don't think this is a safe assumption to make, and in this case, is false. There are no macros anywhere in this test case.

The point is to give the same result as two independent pragma pairs, whose regions do not need to be nested.

I don't find this to be intuitive behavior. These are stack manipulations with the names push and pop -- pretending that they're overlapping rather than a stack in the presence of labels is confusing. If I saw the code from this test case during a code review, I would flag it as being incorrect because the labels do not match -- I don't think I'd be the only one.

erik.pilkington marked an inline comment as done.Dec 14 2018, 9:45 AM
erik.pilkington added inline comments.
clang/test/Sema/pragma-attribute-label.c
7 ↗(On Diff #177971)

My implicit assumption (which I should have been more clear about!) was that you'd only really ever write a label on a push in a BEGIN/END macro. In that world, you'd only ever see this case if 1) you're interacting with another macro that doesn't use the label convention, or 2) if you're interacting with manual push/pop code. In both of those cases, you'd end up popping the wrong attribute group and start applying attributes onto declarations that the programmer didn't intend.

I'm fine with downgrading this to a warning, but IMO an error seems more appropriate. If we wanted to force 1) or 2) through the compiler then we'd also need to downgrade pop UNPUSHED_LABEL to a warning, which doesn't seem like the end of the world either.

15 ↗(On Diff #177971)

I think the labels only really makes sense if you're writing macros that hide the pragma attribute stack (like ASSUME_X_BEGIN/END, for instance), which for better or for worse people do write, and in fact was the intended use case for #pragma clang attribute. I think if we were to write this feature again, we'd forgo the stack entirly and require every push to have a label and be in its own namespace. But this is the best we can do now.

I don't really think that anyone should write a push label outside of a macro definition, since I agree that the semantics are a bit surprising when you're writing the #pragmas yourself without macros. I'll update this test case and the documentation to stress this point more. If you think this is going to be a potential pain point, maybe we can even warn on using a label outside of a macro definition.

erik.pilkington marked 7 inline comments as done.

Address @aaron.ballman comments. Thanks!

clang/docs/LanguageExtensions.rst
2667 ↗(On Diff #177971)

Sure, I moved this to the end of the section, added an example, and left the manual #pragma clang attribute below intact to make it more clear.

clang/include/clang/Basic/DiagnosticSemaKinds.td
840–842 ↗(On Diff #177971)

Sure, good point.

dexonsmith added inline comments.Dec 14 2018, 11:48 AM
clang/test/Sema/pragma-attribute-label.c
7 ↗(On Diff #177971)

I'm not fine with this just being a warning. The entire point is that the stacks are independent; having them suddenly mixed here would be super confusing. @aaron.ballman, please see my other comment for a longer explanation.

15 ↗(On Diff #177971)

The point is to give the same result as two independent pragma pairs, whose regions do not need to be nested.

I don't find this to be intuitive behavior. These are stack manipulations with the names push and pop -- pretending that they're overlapping rather than a stack in the presence of labels is confusing. If I saw the code from this test case during a code review, I would flag it as being incorrect because the labels do not match -- I don't think I'd be the only one.

As Erik says, the intention is that these are only used by macros.

Stepping back, the goal of this pragma (which we added in https://reviews.llvm.org/D30009) is to avoid adding a new region-based pragma every time we want to apply an attribute within a region. Clang has a lot of these pragmas, in order to support independent macros like this:

#define A_BEGIN _Pragma("clang a push")
#define A_END   _Pragma("clang a pop")
#define B_BEGIN _Pragma("clang b push")
#define B_END   _Pragma("clang b pop")
#define C_BEGIN _Pragma("clang c push")
#define C_END   _Pragma("clang c pop")

@arphaman came up with the idea of introduce "one pragma to rule them all", pragma clang attribute, so that we didn't need to introduce a new pragma each time we wanted an independent region:

#define A_BEGIN _Pragma("clang attribute push(a)")
#define A_END   _Pragma("clang attribute pop")
#define B_BEGIN _Pragma("clang attribute push(b)")
#define B_END   _Pragma("clang attribute pop")
#define C_BEGIN _Pragma("clang attribute push(c)")
#define C_END   _Pragma("clang attribute pop")

However, we've realized that there is a major design flaw: these macros are not independent, because they're using the same stack. @erik.pilkington has come up with this solution using identifiers to namespace the attribute stacks, allowing the macros to be independent (like they were before, when each had a dedicated pragma):

#define A_BEGIN _Pragma("clang attribute push A (a)")
#define A_END   _Pragma("clang attribute pop  A")
#define B_BEGIN _Pragma("clang attribute push B (b)")
#define B_END   _Pragma("clang attribute pop  B")
#define C_BEGIN _Pragma("clang attribute push C (c)")
#define C_END   _Pragma("clang attribute pop  C")

To be clear, without this behaviour (or something equivalent) #pragma clang attribute is completely broken for its motivating use case. If we can't find a design that allows us to interleave these macros, we will have to abandon this pragma entirely (and go back to adding a significant number of dedicated one-off pragmas).

But it's the behaviour we care about, not this particular syntax. Since this pragma is designed specifically to be hidden behind macros, it can be as ugly as you want. Is there another way of spelling this that you find more intuitive? Or should we just improve the docs as Erik has done?

aaron.ballman added inline comments.Dec 14 2018, 12:21 PM
clang/test/Sema/pragma-attribute-label.c
15 ↗(On Diff #177971)

As Erik says, the intention is that these are only used by macros.

That's a nice intention, but that doesn't mean all users are going to do that. I'm worried about the users who don't adhere to that intention and use the pragmas in their more exposed format, given that it's a supported syntax.

But it's the behaviour we care about, not this particular syntax. Since this pragma is designed specifically to be hidden behind macros, it can be as ugly as you want.

I'm not worried about the ugliness, I'm worried about whether it's understandable behavior when the pragma is not hidden behind a macro.

Is there another way of spelling this that you find more intuitive? Or should we just improve the docs as Erik has done?

@erik.pilkington and I talked over IRC and I have a better understanding of where my disconnect is here. It seems as though push/pop was the incorrect initial design for the feature and what we really wish we had was a region-based approach rather than a stack-based approach. Would it make sense to add #pragma clang attribute begin/end syntax with labels instead? Then the macros could switch to using that approach to solve the problems you're running into. We could still have push/pop with labels, but they could warn on label mismatches (which would also help alert users to problems like what is in the summary for this patch) instead of treating it as a region.

dexonsmith added inline comments.Dec 14 2018, 12:59 PM
clang/test/Sema/pragma-attribute-label.c
15 ↗(On Diff #177971)

Sounds great; thanks Aaron.

Another idea I had over lunch was #pragma clang attribute IDENTIFIER push/pop, making it clear that each identifier has an independent stack.

After looking through some users of #pragma clang attribute, I don't think that the begin/end solution is what we want here. Some users of this attribute write macros that can expand to different attributes depending on their arguments, for instance:

AVAILABILITY_BEGIN(macos(10.12)) // expands to an availability attribute
AVAILABILITY_BEGIN(ios(10))
// some code...
AVAILABILITY_END
AVAILABILITY_END

This code makes sense and is safe, but in this case rewriting AVAILABILITY_BEGIN to use a hypothetical pragma clang attribute begin/end would be a breaking change, which isn't acceptable. So I think that we want stack semantics, but scoped within the AVAILABILITY_BEGIN macro's namespace. That way, we can support multiple pushes in the same macro, without having having different macros accidentally pop each other.

As for a syntax for this, I chose the following (basically, @dexonsmith's idea with a '.'):

#pragma clang attribute NoReturn.push (__attribute__((noreturn)), apply_to=function)
int foo();
#pragma clang attribute NoReturn.pop

I think the '.' makes the nested relationship (i.e. the push is contained within the namespace) more clear to C programmers, and hopefully clears up the confusion. @AaronBallman: WDYT?

Just to be clear, this is just a syntactic change from what I originally posted. The old #pragma clang attribute push NoReturn (...) is semantically equivalent to the new #pragma clang attribute NoReturn.push (...)

After looking through some users of #pragma clang attribute, I don't think that the begin/end solution is what we want here. Some users of this attribute write macros that can expand to different attributes depending on their arguments, for instance:

AVAILABILITY_BEGIN(macos(10.12)) // expands to an availability attribute
AVAILABILITY_BEGIN(ios(10))
// some code...
AVAILABILITY_END
AVAILABILITY_END

This code makes sense and is safe, but in this case rewriting AVAILABILITY_BEGIN to use a hypothetical pragma clang attribute begin/end would be a breaking change, which isn't acceptable.

Good catch!

So I think that we want stack semantics, but scoped within the AVAILABILITY_BEGIN macro's namespace. That way, we can support multiple pushes in the same macro, without having having different macros accidentally pop each other.

As for a syntax for this, I chose the following (basically, @dexonsmith's idea with a '.'):

#pragma clang attribute NoReturn.push (__attribute__((noreturn)), apply_to=function)
int foo();
#pragma clang attribute NoReturn.pop

I think the '.' makes the nested relationship (i.e. the push is contained within the namespace) more clear to C programmers, and hopefully clears up the confusion. @AaronBallman: WDYT?

This is definitely a novel syntax, but I like how clear it is that the label acts as the scope being pushed and popped. One question I have is with interactions from unlabeled pushes and pops:

#pragma clang attribute MyNamespace.push (__attribute__((annotate)), apply_to=function)
#pragma clang attribute push (__attribute__((annotate)), apply_to=function)

int some_func1();

#pragma clang attribute pop // does this pop the unlabeled push, or is it an error?
#pragma clang attribute MyNamespace.pop

Or, similarly:

#pragma clang attribute push (__attribute__((annotate)), apply_to=function)
#pragma clang attribute MyNamespace.push (__attribute__((annotate)), apply_to=function)

int some_func2();

#pragma clang attribute pop // does this pop the unlabeled push, or is it an error?
#pragma clang attribute MyNamespace.pop

I suspect the answer is that it pops the unlabeled push and is not an error, but I'd like to see a test case with it.

clang/docs/LanguageExtensions.rst
2700 ↗(On Diff #178837)

Somewhere around here you should probably mention that an unlabeled pop does *not* act like a "pop any namespace" action but instead acts as a "pop the last unlabeled namespace" action. Some users may find that behavior surprising otherwise.

2709–2714 ↗(On Diff #178837)

I think it would be useful for the example to show the problematic situation this feature is meant to avoid.

clang/lib/Parse/ParsePragma.cpp
3161 ↗(On Diff #178837)

Can you reuse diag::err_expected_after instead of making a new diagnostic?

erik.pilkington marked 4 inline comments as done.

Add Aaron's testcase, improve the documentation a bit.

clang/lib/Parse/ParsePragma.cpp
3161 ↗(On Diff #178837)

I was kinda concerned about how we would diagnose a case like this:

#pragma clang attribute add (...) // add isn't a thing!

A generic diagnostic about the missing . would be pretty unhelpful. The custom diagnostic reads as "expected '.' after pragma attribute namespace 'add'", which makes how the parser interpreted the code a lot more clear.

aaron.ballman accepted this revision.Thu, Dec 20, 5:17 AM
aaron.ballman marked an inline comment as done.

LGTM!

clang/lib/Parse/ParsePragma.cpp
3161 ↗(On Diff #178837)

Okay, that's a good reason to go with a separate diagnostic, thanks!

This revision is now accepted and ready to land.Thu, Dec 20, 5:17 AM
This revision was automatically updated to reflect the committed changes.