haberman added inline comments.
================ Comment at: clang/include/clang/Basic/AttrDocs.td:458 +same number of arguments as the caller. The types of the return value and all +arguments must be similar, including the implicit "this" argument, if any. +Any variables in scope, including all arguments to the function and the ---------------- aaron.ballman wrote: > It'd be nice if we could nail down "similar" somewhat. I don't know if `int` > and `short` are similar (due to promotions) or `const int` and `int` are > similar, etc. Done. I tried to summarize the C++ concept of "similar" types as defined in https://eel.is/c++draft/conv.qual#2 and implemented in https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#a1b1b3b7a67a30fd817ba85454780d8ad ================ Comment at: clang/lib/Sema/SemaStmt.cpp:596 + if (!CE->getCalleeDecl()) { + assert(hasUncompilableErrorOccurred() && "expected previous error"); + return false; ---------------- rsmith wrote: > A call expression doesn't necessarily have a known callee declaration. I > would expect this assert to fire on a case like: > ``` > void f() { > void (*p)() = f; > [[clang::musttail]] return p(); > } > ``` > We should reject this with a diagnostic. I think this case will work actually, the callee decl in this case is just the function pointer, which seems appropriate and type checks correctly. I added a test for this. ================ Comment at: clang/lib/Sema/SemaStmt.cpp:631 + // Caller is an Obj-C block decl: ^(void) { /* ... */ } + assert(dyn_cast<BlockDecl>(CurContext) && "unexpected decl context"); + Diag(St->getBeginLoc(), diag::err_musttail_from_block_forbidden) << &MTA; ---------------- rjmccall wrote: > rsmith wrote: > > There are a couple of other contexts that can include a return statement: > > the caller could also be an `ObjCMethodDecl` (an Objective-C method) or a > > `CapturedDecl` (the body of a `#pragma omp` parallel region). I'd probably > > use a specific diagnostic ("cannot be used from a block" / "cannot be used > > from an Objective-C function") for the block and ObjCMethod case, and a > > nonsepcific-but-correct "cannot be used from this context" for anything > > else. > Blocks ought to be extremely straightforward to support. Just validate that > the tail call is to a block pointer and then compare the underlying function > types line up in the same way. You will need to be able to verify that there > isn't a non-trivial conversion on the return types, even if the return type > isn't known at this point in the function, but that's a problem in C++ as > well due to lambdas and `auto` deduced return types. > > Also, you can use `isa<...>` for checks like this instead of `dyn_cast<...>`. Tail calls to a block are indeed straightforward and are handled below. This check is for tail calls from a block, which I tried to add support for but didn't have much luck (in particular, during parsing of a block I wasn't able to get good type information for the block). > I'd probably use a specific diagnostic ("cannot be used from a block" / > "cannot be used from an Objective-C function") for the block and ObjCMethod > case, and a nonsepcific-but-correct "cannot be used from this context" for > anything else. I implemented this as requested. I wasn't able to test OpenMP as you apparently can't return from an OpenMP block. ================ 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: > > > rsmith wrote: > > > > 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? > > > You can find that review at https://reviews.llvm.org/D99896. > > Would it be possible to defer that refactoring until after this change is > > in? There are a lot of other issues to resolve on this review as it is, and > > throwing a potential refactoring into the mix is making it a lot harder to > > get this into a state where it can be landed. > > > > Once it's in I'm happy to collaborate on the other review. > I'm fine with that -- my suggestion would be to ignore the template > instantiation validation for the moment (add tests with FIXME comments where > the behavior isn't what you want) and then when I get you the functionality > you need to have more unified checking, you can refactor it at that time. I would strongly prefer to submit correct code (that validates templates) and leave a FIXME to make it pretty, rather than submit pretty code and leave a FIXME to make it correct. 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
