haberman 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:
> 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.
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
