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