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

[Josh Haberman via Phabricator via cfe-commits]

haberman added a comment.

I addressed nearly all of the comments. I have just a few more test cases to add: Obj-C blocks and ARC.

I left one comment open re: a "regular" function. I'll dig into that more when I am adding the last few test cases.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2832
+def err_musttail_only_from_function : Error<
+  "%0 attribute can only be used from a regular function.">;
+def err_musttail_return_type_mismatch : Error<
----------------
aaron.ballman wrote:
> What is a "regular function?"
I may have been trying to distinguish between blocks, or lambdas, I can't exactly remember.

I think I still need to add tests for blocks and Obj-C refcounts. I'm going to leave this comment open for now as a reminder to revisit this.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:5315-5317
+    // TODO(haberman): insert checks/assertions to verify that this early exit
+    // is safe. We tried to verify this in Sema but we should double-check
+    // here.
----------------
aaron.ballman wrote:
> Are you planning to handle this TODO in the patch? If not, can you switch to a FIXME without a name associated with it?
I am interested in feedback on the best way to proceed here.

  - Is my assessment correct that we should have an assertion that validates this?
  - Is such an assertion reasonably feasible to implement?
  - Is it ok to defer with FIXME, or should I try to fix it in this patch?

I've changed it to a FIXME for now.


================
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:
> 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).


================
Comment at: clang/lib/Sema/SemaStmt.cpp:620
+    const CXXMethodDecl *CMD = dyn_cast<CXXMethodDecl>(mem->getMemberDecl());
+    assert(CMD && !CMD->isStatic());
+    CalleeThis = CMD->getThisType()->getPointeeType();
----------------
aaron.ballman wrote:
> Is this assertion valid? Consider:
> ```
> struct S {
>   static int foo();
> };
> 
> int bar() {
>   S s;
>   [[clang::musttail]] return s.foo();
> }
> ```
I have a test for that in `CodeGen/attr-musttail.cpp` (see `Func4()`). It appears that this goes through `FunctionToPointerDecay` and ends up getting handled by the function case below.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:665-666
+    ArrayRef<ParmVarDecl *> caller_params = CallerDecl->parameters();
+    size_t n = CallerDecl->param_size();
+    for (size_t i = 0; i < n; i++) {
+      if (!TypesMatch(callee_params[i], caller_params[i]->getType())) {
----------------
aaron.ballman wrote:
> How do you want to handle variadic function calls?
I added a check to disallow variadic function calls.


================
Comment at: clang/test/Sema/attr-musttail.cpp:17
+long g(int x);
+int h(long x);
+
----------------
aaron.ballman wrote:
> I'd appreciate a test of promotion behavior:
> ```
> int i(int x);
> int s(short x);
> 
> int whatever1(int x) {
>   [[clang::musttail]] return s(x);
> }
> 
> int whatever2(short x) {
>   [[clang::musttail]] return i(x);
> }
> ```
Done (my understanding is that these should both fail, since they would violate the LLVM rules that the caller and callee prototypes must match).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99517