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:
> > haberman wrote:
> > > 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.
> > I'm okay with that so long as the follow-up work actually happens (not to
> > suggest that you plan to ignore the request!). "This is functional but not
> > pretty" has a risk of becoming enshrined behavior as priorities shift,
> > whereas "this is incomplete" generally does not.
> >
> > Please add a FIXME comment here just to make sure it's clear we want the
> > code to move in the future.
> I added a FIXME. Just to set expectations, I'm happy to work with you on
> updating this code to fit your planned refactoring (either by offering
> comments/suggestions on a review by you or creating my own follow-up review
> per your suggestions). But I'll need a fair amount of input from you, since I
> don't fully grok what you find objectionable about the current code or what
> your desired end state is.
Thanks for the FIXME. I'm totally happy to iterate with you on the refactoring.
Mostly, it involves testing whether https://reviews.llvm.org/D99983 provides
you with enough contextual information when performing template instantiation
for you to be able to put the attribute checking logic into the right places.
The objectionable bit about the current approach is that
`ActOnAttributedStmt()`/`BuildAttributedStmt()` are general functions for
attributed statements that should not be doing per-attribute diagnostic work
(this won't scale well as more statement attributes get added). My preferred
approach based on what you have already is to call `checkMustTailAttr()` from
`handleMustTailAttr()`, and call it from `TreeTransform.h` in a new
`TransformMustTailAttr()` function when doing template instantiation (this part
is what requires the other patch to land first).
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
