hokein accepted this revision.
hokein added a comment.
OK, let's move forward with it. Thanks for the investigation and the fix!
I will take a look on the invalid-bit problem, and monitor the crash report
more closely.
================
Comment at: clang/lib/Sema/TreeTransform.h:14708
} else if (Op == OO_Arrow) {
+ if (First->getType()->isDependentType())
+ return ExprError();
----------------
sammccall wrote:
> hokein wrote:
> > I'm not sure this is the only place triggering the crash, it looks like
> > that we're fixing a symptom.
> >
> > While here, `First` refers to a dependent-type `RecoveryExpr` (which is
> > built from the code path: `TransformDeclRefExpr` -> `RebuildDeclRefExpr`->
> > `Sema::BuildDeclarationNameExpr`). So I think we have a high chance that
> > the `RecoveryExpr` will spread multiple places in the `TreeTransform` and
> > cause similar violations in other places.
> >
> > A safe fix will be to not build the `RecoveryExpr` in
> > `TreeTransform::TransformDeclRefExpr` -- `Sema::BuildDeclarationNameExpr`
> > has a `AcceptInvalidDecl` parameter (by default it is false), we could
> > reuse it to control whether we should build a `RecoveryExpr`.
> >
> I agree with this FWIW: in principle it makes sense to have RecoveryExpr
> produced in template instantiation, in practice we probably haven't weakened
> the assumptions in template instantiation enough to do so safely, in the way
> that we have for "regular" sema.
>
> We could try to do so in an ongoing way, but at least for syntax based tools
> like clangd the payoff won't be large as long as we keep mostly ignoring
> template instantiations.
>
> That said, the current form of the patch is simple and fixes the crash in an
> obvious way, if this really is a more general problem then we'll see it again
> and have more data to generalize.
yeah, I'm more worry about this is a more general problem in template
instantiation.
I agree that having RecoveryExpr produced in template instantiation makes
sensible (mostly for diagnostics), but it seems to me that we're opening a can
of worms, and I'm not sure this is a good tradeoff -- from our experience,
tracing and fixing these kind of crashes is quite painful and requires large
amount of effort (personally, I will be more conservative).
Given the current patch is a definitely crash fix, I'm fine with it.
================
Comment at: clang/lib/Sema/TreeTransform.h:14708
} else if (Op == OO_Arrow) {
+ if (First->getType()->isDependentType())
+ return ExprError();
----------------
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > I'm not sure this is the only place triggering the crash, it looks like
> > > that we're fixing a symptom.
> > >
> > > While here, `First` refers to a dependent-type `RecoveryExpr` (which is
> > > built from the code path: `TransformDeclRefExpr` ->
> > > `RebuildDeclRefExpr`-> `Sema::BuildDeclarationNameExpr`). So I think we
> > > have a high chance that the `RecoveryExpr` will spread multiple places in
> > > the `TreeTransform` and cause similar violations in other places.
> > >
> > > A safe fix will be to not build the `RecoveryExpr` in
> > > `TreeTransform::TransformDeclRefExpr` -- `Sema::BuildDeclarationNameExpr`
> > > has a `AcceptInvalidDecl` parameter (by default it is false), we could
> > > reuse it to control whether we should build a `RecoveryExpr`.
> > >
> > I agree with this FWIW: in principle it makes sense to have RecoveryExpr
> > produced in template instantiation, in practice we probably haven't
> > weakened the assumptions in template instantiation enough to do so safely,
> > in the way that we have for "regular" sema.
> >
> > We could try to do so in an ongoing way, but at least for syntax based
> > tools like clangd the payoff won't be large as long as we keep mostly
> > ignoring template instantiations.
> >
> > That said, the current form of the patch is simple and fixes the crash in
> > an obvious way, if this really is a more general problem then we'll see it
> > again and have more data to generalize.
> yeah, I'm more worry about this is a more general problem in template
> instantiation.
> I agree that having RecoveryExpr produced in template instantiation makes
> sensible (mostly for diagnostics), but it seems to me that we're opening a
> can of worms, and I'm not sure this is a good tradeoff -- from our
> experience, tracing and fixing these kind of crashes is quite painful and
> requires large amount of effort (personally, I will be more conservative).
>
> Given the current patch is a definitely crash fix, I'm fine with it.
nit: please add some comments explaining why we hit a dependent-type express
here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121824/new/
https://reviews.llvm.org/D121824
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
