[PATCH] D55628: Add support for "labels" on push/pop directives in #pragma clang attribute

Fri Dec 14 12:59:57 PST 2018

dexonsmith added inline comments.

================
Comment at: clang/test/Sema/pragma-attribute-label.c:15
+// Out of order!
+#pragma clang attribute pop MY_LABEL
+
----------------
aaron.ballman wrote:
> dexonsmith wrote:
> > erik.pilkington wrote:
> > > aaron.ballman wrote:
> > > > dexonsmith wrote:
> > > > > aaron.ballman wrote:
> > > > > > 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.
> > > > > 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.
> > > > > 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.
> > > 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. 
> > >> 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?
> > 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.
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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55628/new/

https://reviews.llvm.org/D55628