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