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

Mon Apr 12 16:17:53 PDT 2021

rsmith added inline comments.

================
Comment at: clang/lib/Sema/SemaStmt.cpp:603-604
+  ReturnStmt *R = cast<ReturnStmt>(St);
+  R->setRetValue(IgnoreImplicitAsWritten(R->getRetValue()));
+  Expr *Ex = R->getRetValue();
+  while (!isa<CallExpr>(Ex)) {
----------------
I think this would be clearer, assuming it's equivalent (and if it's not equivalent, I think it'd be useful to include a comment explaining why).

================
Comment at: clang/lib/Sema/SemaStmt.cpp:605-609
+  while (!isa<CallExpr>(Ex)) {
+    auto *PE = cast<ParenExpr>(Ex);
+    Ex = IgnoreImplicitAsWritten(PE->getSubExpr());
+    PE->setSubExpr(Ex);
+  }
----------------
This loop is problematic: it's generally not safe to modify an expression that is used as a subexpression of another expression. (Modifying the `ReturnStmt` is, by contrast, much less problematic because the properties of a statement have less complex dependencies on the properties of its subexpressions.) In particular, if there were any implicit conversions here that changed the type or value category or similar, the enclosing parentheses would have the wrong type / value category / similar. Also there are possibilities here other than `CallExpr` and `ParenExpr`, such as anything else that we consider to be "parentheses" (such as a `GenericSelectionExpr`).

But I think this loop should never be necessary, because all implicit conversions should always be on the outside of the parentheses. Do you have a testcase that needs it?

================
Comment at: clang/lib/Sema/SemaStmt.cpp:618
+
+  const ReturnStmt *R = cast<const ReturnStmt>(St);
+  const Expr *Ex = R->getRetValue();
----------------
... would be more in line with our normal idioms.

================
Comment at: clang/lib/Sema/SemaStmt.cpp:687
+    // Caller is a non-method function.
+    CallerType.Func = dyn_cast<FunctionProtoType>(CallerDecl->getType());
+  }
----------------
Use `getAs` rather than `dyn_cast` to look through type sugar. For example, in

```
void (f)() { [[clang::musttail]] return f(); }
```
... the type of `f` is a `ParenType`, not a `FunctionProtoType`.

================
Comment at: clang/lib/Sema/SemaStmt.cpp:697
+      return false;
+  } else if (VD && isa<MemberPointerType>(VD->getType())) {
+    // Call is: obj->*method_ptr or obj.*method_ptr
----------------
You need to use `getAs<MemberPointerType>` here not `isa` in order to look through type sugar (eg, typedefs).

However, as noted above, a call via a member pointer doesn't necessarily have a `CalleeDecl`, so you'll need to do this check by looking for a callee expression that's the right kind of `BinaryOperator` instead.

================
Comment at: clang/lib/Sema/SemaStmt.cpp:636-637
+
+  if (!CE->getCalleeDecl()) {
+    assert(hasUncompilableErrorOccurred() && "expected previous error");
+    return false;
----------------
haberman wrote:
> aaron.ballman wrote:
> > This worries me slightly -- not all `CallExpr` objects have a callee declaration (https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1367). That said, I'm struggling to come up with an example that isn't covered so this may be fine.
> That was my experience too, I wasn't able to find a case that isn't covered. I tried to avoid adding any diagnostics that I didn't know how to trigger or test.
This assert is incorrect. It would fail for a case like:

```
using T = int();
T *f();
int g() { [[clang::musttail]] return f()(); }
```

... where there is no declaration associated with the function pointer returned by `f()`.

I think instead of looking for a callee declaration, you should instead inspect the callee expression. You can distinguish between a member function call and a non-member call by looking at the type of the callee. Perhaps the simplest way would be to distinguish between three cases:

(1) There is a callee declaration, which is a member function: this is a direct call to a member function; you can use the type of the callee declaration for your check.
(2) The callee expression is (after skipping parens) a pointer-to-member access operator (`BinaryOperator::isPtrMemOp`); you can use the type of the RHS operand (which will be a pointer to member function) for your check.
(3) Anything else: this is a non-member-function call, and you can directly inspect the type of the callee without caring about the callee declaration. (You might still find the type is not a function type at this stage, which indicates this is some kind of special form. In particular, it could be a `BuiltinType::BoundMember` for a pseudo-destructor call. I'm not sure if there are currently any other special cases that make it this far; there might not be, because most such cases are dependent.)

================
Comment at: clang/test/CodeGen/attr-musttail.cpp:1
+// RUN: %clang_cc1 -fno-elide-constructors -S -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - | FileCheck %s
+// RUN: %clang_cc1 -fno-elide-constructors -S -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - | opt -verify
----------------
This is a C++ test so it should be in `CodeGenCXX`.

================
Comment at: clang/test/CodeGen/attr-musttail.cpp:178-181
+void NoCalleeDecl(int x) {
+  void (*p)(int) = nullptr;
+  [[clang::musttail]] return p(x);
+}
----------------
It turns out that we consider `p` to be the callee decl in this case, so we'll need a better example :)

================
Comment at: clang/test/CodeGen/attr-musttail.cpp:194
+
+// CHECK: musttail call void @_Z12ReturnsLargev(%struct.LargeWithCopyConstructor* sret(%struct.LargeWithCopyConstructor) align 1 %agg.result)
----------------
This doesn't include enough of the output to be able to tell if we've generated correct code. Can you also include the `define ...` line, showing that `%agg.result` is the name of the first parameter?

================
Comment at: clang/test/Sema/attr-musttail.cpp:1
+// RUN: %clang_cc1 -verify -fsyntax-only -fms-extensions -fcxx-exceptions -fopenmp %s
+
----------------
This should be in `SemaCXX`.

================
Comment at: clang/test/Sema/attr-musttail.cpp:66
+HasNonTrivialDestructor TestReturnsNonTrivialValue() {
+  [[clang::musttail]] return (ReturnsNonTrivialValue()); // expected-error {{'musttail' attribute requires that the return value, all parameters, and any temporaries created by the expression are trivially destructible}}
+}
----------------
Please add a FIXME to this; it seems like a bug that we can't tell the difference between needing to run a destructor for the return value and needing to run a destructor for some other temporary created in the return statement.

================
Comment at: clang/test/Sema/attr-musttail.cpp:78
+  // "this" pointer cannot double as first parameter.
+  [[clang::musttail]] return (foo->*(foo->p_mem))(); // expected-error {{'musttail' attribute requires that caller and callee have compatible function signatures}} expected-note{{target function is a member of different class (expected 'void' but has 'UsesPointerToMember')}}
+}
----------------
The "is a member of different class (expected `void`" seems surprising here. Can we customize the diagnostic to instead say that we can't `musttail` from a non-member to a member (and vice versa for the other case)?

================
Comment at: clang/test/Sema/attr-musttail.cpp:167-171
+struct ClassWithDestructor { // expected-note {{callee is a destructor}}
+  void TestExplicitDestructorCall() {
+    [[clang::musttail]] return this->~ClassWithDestructor(); // expected-error {{'musttail' attribute cannot be used when caller or callee is a constructor or destructor, as these can have unusual calling conventions}}
+  }
+};
----------------
Please also test the pseudo-destructor case:
```
void f() {
  int n;
  using T = int;
  [[clang::musttail]] return n.~T();
}
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99517