[PATCH] D86559: [Sema, CodeGen] Allow [[likely]] and [[unlikely]] on labels

Wed Nov 4 06:05:06 PST 2020

aaron.ballman added a comment.

In D86559#2371581 <https://reviews.llvm.org/D86559#2371581>, @Mordante wrote:

> In D86559#2371058 <https://reviews.llvm.org/D86559#2371058>, @aaron.ballman wrote:
>
>> In D86559#2369317 <https://reviews.llvm.org/D86559#2369317>, @Mordante wrote:
>>
>>> Then me try to clear up the confusion.
>>>
>>>> However, I could imagine there being cases when we might want a helper function on `LabelDecl` that looks for attributes on the associated `LabelStmt` and returns results about it if that helps ease implementation or refactoring burdens.
>>>
>>> If we want that we need to change the `LabelDecl` to point to either a `LabelStmt` or an `AttributedStmt`. This was the approach I thought I had to take, but I found this solution. We can still take that direction if wanted. But then we need to place `DeclAttr` in an `AttributedStmt`, not sure how well that works and how much additional code needs to change to find the attributes there. Since in that case we to call this helper function at the appropriate places.
>>
>> Ah, I was thinking of something slightly different here. I was thinking that `LabelDecl` would hold a `Stmt*` so that it could be either a label statement or an attribute statement.
>
> Yes I wanted to take that route. While investigating that route I found the current solution and it seemed less intrusive. So that's why I went for the current approach.
>
>> The helper functions would give you access to the attributes of the statement and to the `LabelStmt` itself (stripping off any attributed statements). Then in Attr.td, we'd move attributes off the label declarations and onto the label statements. At the end of the day, all attributes on labels would appertain to the statement at the AST level, but you'd have an easier time transitioning in some places if you could get to the attributes if the only thing you have is the `LabelDecl`. (So this doesn't mix statement and declaration attributes together, it shifts the model to putting all attributes on labels on the statement level rather than having a somewhat odd split between decl and statement.)
>
> If we go that route, then we need to think about attributes that are normally allowed on declarations. For example
>
>   def Unused : InheritableAttr {
>     let Spellings = [CXX11<"", "maybe_unused", 201603>, GCC<"unused">,
>                      C2x<"", "maybe_unused", 201904>];
>     let Subjects = SubjectList<[Var, ObjCIvar, Type, Enum, EnumConstant, Label,
>                                 Field, ObjCMethod, FunctionLike]>;
>     let Documentation = [WarnMaybeUnusedDocs];
>   }

Yup, `annotate` is another such attribute, to some degree. I was envisioning that these tablegen definitions would have to change slightly to allow for writing on a statement for the label case.

> It also seems attributes on the `LabelDecl` aren't visible in the AST at the moment. If I modify the example posted yesterday to:
>
>   void foo() {
>     [[likely, clang::annotate("foo")]] lbl:;
>   }
>
> The AST will become:
>
>   `-FunctionDecl 0x5607f7dbb7a8 </tmp/x.cpp:1:1, line:3:1> line:1:6 foo 'void ()'
>     `-CompoundStmt 0x5607f7dbba60 <col:12, line:3:1>
>       `-AttributedStmt 0x5607f7dbba48 <line:2:3, col:42>
>         |-LikelyAttr 0x5607f7dbba20 <col:5>
>         `-LabelStmt 0x5607f7dbba08 <col:38, col:42> 'lbl'
>           `-NullStmt 0x5607f7dbb928 <col:42>
>
> Maybe it would be a good idea to see whether the `LabelDecl` needs to be visible in the AST even if we proceed with the current approach.

Good catch, I hadn't noticed that before. I agree, we should figure that out. Some simple testing shows clang-query also doesn't match label declarations with the `decl()` matcher, so the issue is wider than just dumping the AST.

>>> Does this clear your confusion?
>>> Do you agree with this approach or do you think changing the `LabelDecl` is the better solution?
>>
>> Thank you for the explanations, I understand your design better now. I'm not certain what the right answer is yet, but thinking out loud about my concerns: I worry that making a distinction between a label statement and a label declaration (at least for attributes) generates heat without light. Making the attribute author decide "which label construct should my attribute appertain to" adds extra burden on attribute authors and I'm not sure I have any advice for them on whether to use the decl or the statement -- it seems entirely arbitrary. Coupled with the fact that the standard defines labels as being statements, that suggests to me that putting all of the attributes on the statement level is the right decision -- it's one place (removes confusion about where it goes), it's the "obvious" place (matches where the standard suggests that attributes live), and we should have all the machinery in place to make it possible within the compiler (given that you can reach the `LabelStmt` from the `LabelDecl`).
>
> I agree it's not clear cut what the best way is. For example `unused` is a declaration attribute, but `likely` is a statement attribute. Both can be used on a label.

`unused` is also a bit weird because `[[maybe_unused]]` technically does not apply to labels (http://eel.is/c++draft/dcl.attr.unused#2, we are using a conforming extension in our implementation), but `__attribute__((unused))` does apply to labels.

> I think for authors in most cases it will be clear, their attribute is also used on other declarations or statements. The harder case will be, when a label specific attribute is added, should it then go on the declaration or the statement. I foresee another issue when there will be attributes that can be attached to both a declaration and a statement, for example if we implement `[[clang::suppress("compiler.warning.12345")]]`. (But I suspect that attribute has more implementation challenges.) In the current approach that attribute will be attached to the declaration since that's parsed first. I think regardless of the final solution labels will remain a special case for attributes.

I'm more and more thinking of `LabelDecl` as an implementation detail that allows us to perform lookups easier rather than a salient piece of the AST, so I guess I view label-specific attributes as being the easy case where the attributes consistently go on the statement level.

> During the review of `[[clang::nomerge]]` the placement on labels was also discussed. I also see some threads on the dev-ml about making more use of attributes. For example to enhance the analyzers, which sounds very interesting. So I feel it's worth the effort to look for the "best" approach, even if the usefulness for the likelihood attributes is limited.

I agree and I greatly appreciate your patience while we work the design out.

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

https://reviews.llvm.org/D86559