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

[Josh Haberman via Phabricator via cfe-commits]

haberman added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:588
+
+  const CallExpr *CE = dyn_cast<CallExpr>(Ex->IgnoreUnlessSpelledInSource());
+
----------------
rsmith wrote:
> `IgnoreUnlessSpelledInSource` is a syntactic check that's only really intended for tooling use cases; I think we want something a bit more semantic here, so `IgnoreImplicitAsWritten` would be more appropriate.
> 
> I think it would be reasonable to also skip "parentheses" here (which we treat as also including things like C's `_Generic`). Would `Ex->IgnoreImplicitAsWritten()->IgnoreParens()` work?
> 
> If we're going to skip elidable copy construction of the result here (which I think we should), should we also reflect that in the AST? Perhaps we should strip the return value down to being just the call expression? I'm thinking in particular of things like building in C++14 or before with `-fno-elide-constructors`, where code generation for a by-value return of a class object will synthesize a local temporary to hold the result, with a final destination copy emitted after the call. (Testcase: `struct A { A(const A&); }; A f(); A g() { [[clang::musttail]] return f(); }` with `-fno-elide-constructors`.)
`IgnoreImplicitAsWritten()` doesn't skip `ExprWithCleanups`, and per your previous comment I was trying to find a `CallExpr` before doing the check prohibiting `ExprWithCleanups` with side effects.

I could write some custom ignore logic using `clang::IgnoreExprNodes()` directly.

> If we're going to skip elidable copy construction of the result here (which I think we should)

To clarify, are you suggesting that we allow `musttail` through elidable copy constructors on the return value, even if `-fno-elide-constructors` is set? ie. we consider that `musttail` overrides the `-fno-elide-constructors` option on the command line?


================
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();
+    }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99517