aaron.ballman added inline comments.
================ 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: > > > 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. 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