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();
+ }
----------------
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.
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
