haberman marked 2 inline comments as done.
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:
> 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).
Sounds good. I will follow up with you on https://reviews.llvm.org/D99983.
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
