[PATCH] D121824: [clang] Do not crash on arrow operator on dependent type.

[Haojian Wu via Phabricator via cfe-commits]

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