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

[Mark de Wever via Phabricator via cfe-commits]

Mordante planned changes to this revision.
Mordante marked 2 inline comments as done.
Mordante added a comment.

Then me try to clear up the confusion.
The parser first parses the `LabelDecl` and tries to attach the attributes to this declaration. If that fails instead of issue a diagnostic for a `StmtAttr` attached to a declaration it let's them be attached to the `LabelStmt`. This way attributes can be placed on both the `LabelDecl` and the `LabelStmt`. For example:

  void foo() {
    __label__ lbl; // Needed to show the LabelDecl in the AST
    [[likely, clang::annotate("foo")]] lbl:;
  }

Will result in the following AST

  `-FunctionDecl 0x560be00b6728 </tmp/x.cpp:1:1, line:4:1> line:1:6 foo 'void ()'
    `-CompoundStmt 0x560be00b6a00 <col:12, line:4:1>
      |-DeclStmt 0x560be00b6860 <line:2:3, col:16>
      | `-LabelDecl 0x560be00b6810 <col:3, col:13> col:13 lbl
      |   `-AnnotateAttr 0x560be00b6920 <line:3:13, col:34> "foo"
      |     `-StringLiteral 0x560be00b68f8 <col:29> 'const char [4]' lvalue "foo"
      `-AttributedStmt 0x560be00b69e8 <col:3, col:42>
        |-LikelyAttr 0x560be00b69c0 <col:5>
        `-LabelStmt 0x560be00b69a8 <col:38, col:42> 'lbl'
          `-NullStmt 0x560be00b6918 <col:42>

I thought this was a sensible approach where I don't change the behaviour of `DeclAttr` on the `LabelDecl`, but at the same time allow the `StmtAttr` to be "forwarded" to the `LabelStmt`, turning it into an `AttributedStmt` with the `LabelStmt` as substatement.

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

Does this clear your confusion?
Do you agree with this approach or do you think changing the `LabelDecl` is the better solution?



================
Comment at: clang/include/clang/Sema/Sema.h:4528
+  StmtResult ActOnLabelStmt(SourceLocation IdentLoc,
+                            const ParsedAttributesView *Attrs,
+                            SourceRange AttrsRange, LabelDecl *TheDecl,
----------------
aaron.ballman wrote:
> Don't we have a `ParsedAttributesViewWithRange` (or something along those lines) that could be used instead of separating attributes and their source range?
Yes there is. I initially used it, but at some point I changed it. I ran into some issues with it in an earlier iteration of the patch. I can't recall the exact details. I'll test again with the version with range.

Note since the new arguments aren't at the end, thus not defaulted the template rebuild call also needs to be changed.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:677
+  }
+  SourceRange AttributeRange = attrs.Range;
   attrs.clear();
----------------
aaron.ballman wrote:
> I think I would have expected this to be calling `Actions.ProcessStmtAttributes()` as done a few lines up on 651 rather than changing the interface to `ActOnLabelStmt()`.
An interesting suggestion. I haven't tried that approach. I'll have to test whether it works. Since the `ActOnLabelStmt` uses the `LabelDecl` I think we can't do the same as on line 651. Maybe something like:
- First call `ActOnLabelStmt` without the attributes,
- If the result is valid and we have attributes call `ProcessStmtAttributes`

The difference with 651 is where the attributes are placed:
```
void foo() {
__label__ L; // Needed to show the LabelDecl in the AST
L: __attribute__((unused));
```
Results in this AST
```
`-FunctionDecl 0x55e1debc1728 </tmp/x.cpp:1:1, line:4:1> line:1:6 foo 'void ()'
  `-CompoundStmt 0x55e1debc18f0 <col:12, line:4:1>
    |-DeclStmt 0x55e1debc1860 <line:2:1, col:12>
    | `-LabelDecl 0x55e1debc1810 <col:1, col:11> col:11 L
    |   `-UnusedAttr 0x55e1debc1880 <line:3:19> unused
    `-LabelStmt 0x55e1debc18d8 <col:1, col:27> 'L'
      `-NullStmt 0x55e1debc1878 <col:27>
```
This is the GNU label attribute extension https://gcc.gnu.org/onlinedocs/gcc/Label-Attributes.html


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7368
+    // The statement attributes attached to a LabelDecl are handled separately.
+    if (!isa<LabelDecl>(D))
+      S.Diag(AL.getLoc(), diag::err_stmt_attribute_invalid_on_decl)
----------------
aaron.ballman wrote:
> This also surprises me -- I wouldn't have expected the attribute to be in the list for the label decl in the first place.
I was also a bit surprised, but the `LabelDecl` is parsed first and the attributes are attached to it. Probably done since it's already possible to attach attributes to a `LabelDecl`.


================
Comment at: clang/lib/Sema/TreeTransform.h:1307
+    // already properly rebuild. Only the LabelStmt itself needs to be rebuild.
+    return SemaRef.ActOnLabelStmt(IdentLoc, nullptr, SourceLocation(), L,
+                                  ColonLoc, SubStmt);
----------------
aaron.ballman wrote:
> I wouldn't have expected any changes to be needed here because the attributed statement should be rebuilt properly by `RebuildAttributedStmt()`, no?
You're correct. There are no real changes here. I added a comment explaining why no changes are needed. Since the signature of `ActOnLabelStmt` has changed, I only call the function using its new interface, but without attributes.


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

https://reviews.llvm.org/D86559