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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
