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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:588
+
+  const CallExpr *CE = dyn_cast<CallExpr>(Ex->IgnoreUnlessSpelledInSource());
+
----------------
haberman wrote:
> 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?
> `IgnoreImplicitAsWritten()` doesn't skip `ExprWithCleanups`

That sounds like a bug. Are you sure? It looks like `IgnoreImplicitAsWrittenSingleStep` calls `IgnoreImplicitSingleStep` which calls `IgnoreImplicitCastsSingleStep` which skips `FullExpr`, and `ExprWithCleanups` is a kind of `FullExpr`.

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

Yes, I think the `musttail` attribute should override `-fno-elide-constructors`, because that's necessary in order to provide the tail call the user requested (and the local setting should override the global one). This is probably worth adding to the documentation.

(Also, `-fno-elide-constructors` is only supposed to affect code generation, not language semantics or program validity, so I think either we should always reject if a constructor call is required for the return value, regardless of whether it's elidable, or we should never reject in that case, and either way this determination should be made independent of the setting of `-fno-elide-constructors`. Given that choice, it seems more useful to bias towards the common case (`-felide-constructors`).)


================
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:
> aaron.ballman wrote:
> > haberman wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > > I think the correct answer is to switch the logic so that ActOnAttributedStmt() calls ProcessStmtAttributes()
> > > 
> > > I think this would require `ProcessStmtAttributes()` to be split into two separate functions. Currently that function is doing two separate things:
> > > 
> > > 1. Translation of `ParsedAttr` into various subclasses of `Attr`.
> > > 2. Validation that the attribute is semantically valid.
> > > 
> > > The function signature for `ActOnAttributedStmt()` uses `Attr` (not `ParsedAttr`), so (1) must happen during the parse, before `ActOnAttributedStmt()` is called. But (2) must be deferred until template instantiation time for some cases, like `musttail`.
> > I don't think the signature for `ActOnAttributedStmt()` is correct to use `Attr` instead of `ParsedAttr`. I think it should be `StmtResult ActOnAttributedStmt(const ParsedAttributesViewWithRange &AttrList, Stmt *SubStmt);` -- this likely requires a fair bit of surgery to make work though, which is why I'd like to hear from @rsmith if he agrees with the approach. In the meantime, I'll play around with this idea locally in more depth.
> I think my suggestion wasn't quite right, but close. I've got a patch in progress that changes this the way I was thinking it should be changed, but it won't call `ActOnAttributedStmt()` when doing template instantiation. Instead, it will continue to instantiate attributes explicitly by calling `TransformAttr()` and any additional instantiation time checks will require you to add a `TreeTransfor::TransformWhateverAttr()` to do the actual instantiation work (which is similar to how the declaration attributes work in `Sema::InstantiateAttrs()`).
> 
> I hope to put up a patch for review for these changes today or tomorrow. It'd be interesting to know whether they make your life easier or harder though, if you don't mind taking a look and seeing how well (or poorly) they integrate with your changes here.
I think the ideal model would be that we form a `FooAttr` from the user-supplied attribute description in an `ActOn*` function from the parser, and have a separate template instantiation mechanism to instantiate `FooAttr` objects, and those methods are unaware of the subject of the attribute. Then we have a separate mechanism to attach an attribute to its subjects that is used by both parsing and template instantiation. But I suspect there are reasons that doesn't work in practice -- where we need to know something about the subject in order to know how to form the `FooAttr`. That being the case, it probably makes most sense to model the formation and application of a `FooAttr` as a single process.

> it won't call `ActOnAttributedStmt()` when doing template instantiation

Good -- not calling `ActOn*` during template instantiation is the right choice in general -- the `ActOn*` functions are only supposed to be called from parsing, with a `Build*` added if the parsing and template instantiation paths would share code (we sometimes shortcut that when the `ActOn*` and `Build*` would be identical, but I think that's turned out to be a mistake).

> any additional instantiation time checks will require you to add a `TreeTransform::TransformWhateverAttr()` to do the actual instantiation work

That sounds appropriate to me in general. Are you expecting that this function would also be given the (transformed and perhaps original) subject of the attribute?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99517