[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:561-568
+  for (const auto *A : Attrs) {
+    if (A->getKind() == attr::MustTail) {
+      if (!checkMustTailAttr(SubStmt, *A)) {
+        return SubStmt;
+      }
+      setFunctionHasMustTail();
+    }
----------------
haberman wrote:
> haberman wrote:
> > aaron.ballman wrote:
> > > haberman wrote:
> > > > aaron.ballman wrote:
> > > > > This functionality belongs in SemaStmtAttr.cpp, I think.
> > > > That is where I had originally put it, but that didn't work for templates. The semantic checks can only be performed at instantiation time. `ActOnAttributedStmt` seems to be the right hook point where I can evaluate the semantic checks for both template and non-template functions (with template functions getting checked at instantiation time).
> > > I disagree that `ActOnAttributedStmt()` is the correct place for this checking -- template checking should occur when the template is instantiated, same as happens for declaration attributes. I'd like to see this functionality moved to SemaStmtAttr.cpp. Keeping the attribute logic together and following the same patterns is what allows us to tablegenerate more of the attribute logic. Statement attributes are just starting to get more such automation.
> > I tried commenting out this code and adding the following code into `handleMustTailAttr()` in `SemaStmtAttr.cpp`:
> > 
> > ```
> >   if (!S.checkMustTailAttr(St, MTA))
> >     return nullptr;
> > ```
> > 
> > This caused my test cases related to templates to fail. It also seemed to break test cases related to `JumpDiagnostics`. My interpretation of this is that `handleMustTailAttr()` is called during parsing only, and cannot catch errors at template instantiation time or that require a more complete AST.
> > 
> > What am I missing? Where in SemaStmtAttr.cpp are you suggesting that I put this check?
> Scratch the part about `JumpDiagnostics`, that was me failing to call `S.setFunctionHasMustTail()`. I added that and now the `JumpDiagnostics` tests pass.
> 
> But the template test cases still fail, and I can't find any hook point in `SemaStmtAttr.cpp` that will let me evaluate these checks at template instantiation time.
I think there's a bit of an architectural mixup, but I'm curious if @rsmith agrees before anyone starts doing work to make changes.

When transforming declarations, `RebuildWhatever()` calls the `ActOnWhatever()` function which calls `ProcessDeclAttributeList()` so that attributes are processed. `RebuildAttributedStmt()` similarly calls `ActOnAttributedStmt()`. However, `ActOnAttributedStmt()` doesn't call `ProcessStmtAttributes()` -- the logic is reversed so that `ProcessStmtAttributes()` is what calls `ActOnAttributedStmt()`.

I think the correct answer is to switch the logic so that `ActOnAttributedStmt()` calls `ProcessStmtAttributes()`, then the template logic should automatically work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99517