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

[Adam Czachorowski via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7e459126185f: [clang] Do not crash on arrow operator on 
dependent type. (authored by adamcz).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121824/new/

https://reviews.llvm.org/D121824

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/arrow-operator.cpp


Index: clang/test/SemaCXX/arrow-operator.cpp
===
--- clang/test/SemaCXX/arrow-operator.cpp
+++ clang/test/SemaCXX/arrow-operator.cpp
@@ -65,3 +65,51 @@
 }
 
 } // namespace arrow_suggest
+
+namespace no_crash_dependent_type {
+
+template 
+struct A {
+  void call();
+  A *operator->();
+};
+
+template 
+void foo() {
+  // The "requires an initializer" error seems unnecessary.
+  A  = blah[7]; // expected-error {{use of undeclared identifier 
'blah'}} \
+// expected-error {{requires an initializer}}
+  // x is dependent.
+  x->call();
+}
+
+void test() {
+  foo(); // expected-note {{requested here}}
+}
+
+} // namespace no_crash_dependent_type
+
+namespace clangd_issue_1073_no_crash_dependent_type {
+
+template  struct Ptr {
+  T *operator->();
+};
+
+struct Struct {
+  int len;
+};
+
+template 
+struct TemplateStruct {
+  Ptr val(); // expected-note {{declared here}}
+};
+
+template 
+void templateFunc(const TemplateStruct ) {
+  Ptr ptr = ts.val(); // expected-error {{function is not marked 
const}}
+  auto foo = ptr->len;
+}
+
+template void templateFunc<0>(const TemplateStruct<0> &); // expected-note 
{{requested here}}
+
+} // namespace clangd_issue_1073_no_crash_dependent_type
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14757,6 +14757,10 @@
   return getSema().CreateBuiltinArraySubscriptExpr(
   First, Callee->getBeginLoc(), Second, OpLoc);
   } else if (Op == OO_Arrow) {
+// It is possible that the type refers to a RecoveryExpr created earlier
+// in the tree transformation.
+if (First->getType()->isDependentType())
+  return ExprError();
 // -> is never a builtin operation.
 return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {


Index: clang/test/SemaCXX/arrow-operator.cpp
===
--- clang/test/SemaCXX/arrow-operator.cpp
+++ clang/test/SemaCXX/arrow-operator.cpp
@@ -65,3 +65,51 @@
 }
 
 } // namespace arrow_suggest
+
+namespace no_crash_dependent_type {
+
+template 
+struct A {
+  void call();
+  A *operator->();
+};
+
+template 
+void foo() {
+  // The "requires an initializer" error seems unnecessary.
+  A  = blah[7]; // expected-error {{use of undeclared identifier 'blah'}} \
+// expected-error {{requires an initializer}}
+  // x is dependent.
+  x->call();
+}
+
+void test() {
+  foo(); // expected-note {{requested here}}
+}
+
+} // namespace no_crash_dependent_type
+
+namespace clangd_issue_1073_no_crash_dependent_type {
+
+template  struct Ptr {
+  T *operator->();
+};
+
+struct Struct {
+  int len;
+};
+
+template 
+struct TemplateStruct {
+  Ptr val(); // expected-note {{declared here}}
+};
+
+template 
+void templateFunc(const TemplateStruct ) {
+  Ptr ptr = ts.val(); // expected-error {{function is not marked const}}
+  auto foo = ptr->len;
+}
+
+template void templateFunc<0>(const TemplateStruct<0> &); // expected-note {{requested here}}
+
+} // namespace clangd_issue_1073_no_crash_dependent_type
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14757,6 +14757,10 @@
   return getSema().CreateBuiltinArraySubscriptExpr(
   First, Callee->getBeginLoc(), Second, OpLoc);
   } else if (Op == OO_Arrow) {
+// It is possible that the type refers to a RecoveryExpr created earlier
+// in the tree transformation.
+if (First->getType()->isDependentType())
+  return ExprError();
 // -> is never a builtin operation.
 return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added a comment.

Thanks for all the comments!


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

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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz updated this revision to Diff 418219.
adamcz marked an inline comment as done.
adamcz added a comment.

added a comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121824/new/

https://reviews.llvm.org/D121824

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/arrow-operator.cpp


Index: clang/test/SemaCXX/arrow-operator.cpp
===
--- clang/test/SemaCXX/arrow-operator.cpp
+++ clang/test/SemaCXX/arrow-operator.cpp
@@ -65,3 +65,51 @@
 }
 
 } // namespace arrow_suggest
+
+namespace no_crash_dependent_type {
+
+template 
+struct A {
+  void call();
+  A *operator->();
+};
+
+template 
+void foo() {
+  // The "requires an initializer" error seems unnecessary.
+  A  = blah[7]; // expected-error {{use of undeclared identifier 
'blah'}} \
+// expected-error {{requires an initializer}}
+  // x is dependent.
+  x->call();
+}
+
+void test() {
+  foo(); // expected-note {{requested here}}
+}
+
+} // namespace no_crash_dependent_type
+
+namespace clangd_issue_1073_no_crash_dependent_type {
+
+template  struct Ptr {
+  T *operator->();
+};
+
+struct Struct {
+  int len;
+};
+
+template 
+struct TemplateStruct {
+  Ptr val(); // expected-note {{declared here}}
+};
+
+template 
+void templateFunc(const TemplateStruct ) {
+  Ptr ptr = ts.val(); // expected-error {{function is not marked 
const}}
+  auto foo = ptr->len;
+}
+
+template void templateFunc<0>(const TemplateStruct<0> &); // expected-note 
{{requested here}}
+
+} // namespace clangd_issue_1073_no_crash_dependent_type
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14757,6 +14757,10 @@
   return getSema().CreateBuiltinArraySubscriptExpr(
   First, Callee->getBeginLoc(), Second, OpLoc);
   } else if (Op == OO_Arrow) {
+// It is possible that the type refers to a RecoveryExpr created earlier
+// in the tree transformation.
+if (First->getType()->isDependentType())
+  return ExprError();
 // -> is never a builtin operation.
 return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {


Index: clang/test/SemaCXX/arrow-operator.cpp
===
--- clang/test/SemaCXX/arrow-operator.cpp
+++ clang/test/SemaCXX/arrow-operator.cpp
@@ -65,3 +65,51 @@
 }
 
 } // namespace arrow_suggest
+
+namespace no_crash_dependent_type {
+
+template 
+struct A {
+  void call();
+  A *operator->();
+};
+
+template 
+void foo() {
+  // The "requires an initializer" error seems unnecessary.
+  A  = blah[7]; // expected-error {{use of undeclared identifier 'blah'}} \
+// expected-error {{requires an initializer}}
+  // x is dependent.
+  x->call();
+}
+
+void test() {
+  foo(); // expected-note {{requested here}}
+}
+
+} // namespace no_crash_dependent_type
+
+namespace clangd_issue_1073_no_crash_dependent_type {
+
+template  struct Ptr {
+  T *operator->();
+};
+
+struct Struct {
+  int len;
+};
+
+template 
+struct TemplateStruct {
+  Ptr val(); // expected-note {{declared here}}
+};
+
+template 
+void templateFunc(const TemplateStruct ) {
+  Ptr ptr = ts.val(); // expected-error {{function is not marked const}}
+  auto foo = ptr->len;
+}
+
+template void templateFunc<0>(const TemplateStruct<0> &); // expected-note {{requested here}}
+
+} // namespace clangd_issue_1073_no_crash_dependent_type
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14757,6 +14757,10 @@
   return getSema().CreateBuiltinArraySubscriptExpr(
   First, Callee->getBeginLoc(), Second, OpLoc);
   } else if (Op == OO_Arrow) {
+// It is possible that the type refers to a RecoveryExpr created earlier
+// in the tree transformation.
+if (First->getType()->isDependentType())
+  return ExprError();
 // -> is never a builtin operation.
 return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Oops, forgot conclusion:
Thanks for incorporating my testcase, and the patch LGTM in its current form 
(most of the alternatives discussed also sound OK).
I'll leave to Haojian to stamp.


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: rsmith.
sammccall added a comment.

In D121824#3400720 , @hokein wrote:

> Sorry, I thought this crash is fixed as a bonus effort by improving the AST 
> for reference-type var-decl.
>
> Beyond the crash cased by the dependent-type `RecoveryExpr` built in 
> `Sema::BuildDeclarationNameExpr`, I think there is another issue which is 
> worth fixing: whether we should mark the reference-type var-decl valid if it 
> doesn't have an initializer (either the initializer is not provided in the 
> source code or too broken to preserve). The current AST behavior is 
> inconsistent:

Agree about the inconsistency. I think ideally we'd move in the direction of 
*not* marking such things invalid. Here's the quote from Richard:

In D76831#1973053 , @rsmith wrote:

> Generally I think we should be moving towards finer-grained "invalid" / 
> "contains errors" bits, so that we can preserve as much of the AST as 
> possible and produce accurate diagnostics for independent errors wherever 
> possible. To that end, I think generally the "invalid" bit on a declaration 
> should concern *only* the "declaration" part of that declaration (how it's 
> seen from other contexts that don't "look too closely"). So in particular:
>
> - The initializer of a variable should play no part in its "invalid" bit. If 
> the initializer expression is marked as "contains errors", then things that 
> care about the initializer (in particular, constant evaluation and any 
> diagnostics that look into the initializer) may need to bail out, but we 
> should be able to form a `DeclRefExpr` referring to that variable -- even if 
> (say) the variable is `constexpr`. Exception: if the variable has a deduced 
> type and the type can't be deduced due to an invalid initializer, then the 
> declaration should be marked invalid.
> - The body of a function should play no part in its "invalid" bit. (This came 
> up in a different code review recently.)

But practically, it's at least as important to make changes that make 
diagnostics better and not worse, and don't introduce new crashes. This is hard 
to do while keeping scope limited, so I'm OK with bending principle too.

> Another option would be to mark `a` valid regardless of the initializer. 
> Pros: preserve more AST nodes for `a`; the valid bit are consistent among 
> three cases. Cons: unknown? whether it'll break some subtle invariants in 
> clang; the bogus `requires an initializer` diagnostic is still there.
> But yeah, this is a separate issue, we should fix the crash first.

+1




Comment at: clang/lib/Sema/TreeTransform.h:14708
   } else if (Op == OO_Arrow) {
+if (First->getType()->isDependentType())
+  return ExprError();

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.


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

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Sorry, I thought this crash is fixed as a bonus effort by improving the AST for 
reference-type var-decl.

Beyond the crash cased by the dependent-type `RecoveryExpr` built in 
`Sema::BuildDeclarationNameExpr`, I think there is another issue which is worth 
fixing: whether we should mark the reference-type var-decl valid if it doesn't 
have an initializer (either the initializer is not provided in the source code 
or too broken to preserve). The current AST behavior is inconsistent:

  int  // var-decl a is invalid without an initializer
  int  = undefine[]; // var-decl b is valid without an initializer
  int  = undefine; // var-decl c is valid with a recovery-expr initializer

From the AST point of view, there is no difference between `a` and `b`, they 
both don't have an initializer, but their valid bits are different. IMO marking 
b invalid is an improvement, the valid bit is consistent for refer-type 
var-decls without initializers; it avoids the bogus `requires an initializer` 
diagnostic during template instantiations; as a bonus, it seems to "fix" the 
crash (at least for the original testcase).

Another option would be to mark `a` valid regardless of the initializer. Pros: 
preserve more AST nodes for `a`; the valid bit are consistent among three 
cases. Cons: unknown? whether it'll break some subtle invariants in clang; the 
bogus `requires an initializer` diagnostic is still there.

But yeah, this is a separate issue, we should fix the crash first.




Comment at: clang/lib/Sema/TreeTransform.h:14708
   } else if (Op == OO_Arrow) {
+if (First->getType()->isDependentType())
+  return ExprError();

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


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

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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added a comment.

In D121824#3399208 , @sammccall wrote:

> I reduced something very similar recently as 
> https://github.com/clangd/clangd/issues/1073
>
> This patch does not fix it, but looks closely related, want to take a look?

Very similar. My original fix would've fixed that too. I'm reverting this 
change to that version and adding this as a test.

Based on my conversation with Sam I'm also reverting the 
mark-ref-VarDecl-as-invalid-when-on-invalid-init, as I no longer believe this 
is appropriate. The idea is that, init or not, we know the type of VarDecl so 
it's not invalid (unless it's auto or something).

PTAL


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

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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz updated this revision to Diff 417331.
adamcz added a comment.

Reverted to previous version + new test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121824/new/

https://reviews.llvm.org/D121824

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/arrow-operator.cpp


Index: clang/test/SemaCXX/arrow-operator.cpp
===
--- clang/test/SemaCXX/arrow-operator.cpp
+++ clang/test/SemaCXX/arrow-operator.cpp
@@ -65,3 +65,51 @@
 }
 
 } // namespace arrow_suggest
+
+namespace no_crash_dependent_type {
+
+template
+struct A {
+  void call();
+  A* operator->();
+};
+
+template 
+void foo() {
+  // The "requires an initializer" error seems unnecessary.
+  A& x = blah[7];  // expected-error {{use of undeclared identifier 
'blah'}} \
+// expected-error {{requires an initializer}}
+  // x is dependent.
+  x->call();
+}
+
+void test() {
+  foo();  // expected-note {{requested here}}
+}
+
+} // namespace no_crash_dependent_type
+
+namespace clangd_issue_1073_no_crash_dependent_type {
+
+template  struct Ptr {
+  T *operator->();
+};
+
+struct Struct {
+  int len;
+};
+
+template 
+struct TemplateStruct {
+  Ptr val();  // expected-note {{declared here}}
+};
+
+template 
+void templateFunc(const TemplateStruct ) {
+  Ptr ptr = ts.val(); // expected-error {{function is not marked 
const}}
+  auto foo = ptr->len;
+}
+
+template void templateFunc<0>(const TemplateStruct<0> &);  // expected-note 
{{requested here}}
+
+} // namespace clangd_issue_1073_no_crash_dependent_type
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14705,6 +14705,8 @@
   return getSema().CreateBuiltinArraySubscriptExpr(
   First, Callee->getBeginLoc(), Second, OpLoc);
   } else if (Op == OO_Arrow) {
+if (First->getType()->isDependentType())
+  return ExprError();
 // -> is never a builtin operation.
 return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {


Index: clang/test/SemaCXX/arrow-operator.cpp
===
--- clang/test/SemaCXX/arrow-operator.cpp
+++ clang/test/SemaCXX/arrow-operator.cpp
@@ -65,3 +65,51 @@
 }
 
 } // namespace arrow_suggest
+
+namespace no_crash_dependent_type {
+
+template
+struct A {
+  void call();
+  A* operator->();
+};
+
+template 
+void foo() {
+  // The "requires an initializer" error seems unnecessary.
+  A& x = blah[7];  // expected-error {{use of undeclared identifier 'blah'}} \
+// expected-error {{requires an initializer}}
+  // x is dependent.
+  x->call();
+}
+
+void test() {
+  foo();  // expected-note {{requested here}}
+}
+
+} // namespace no_crash_dependent_type
+
+namespace clangd_issue_1073_no_crash_dependent_type {
+
+template  struct Ptr {
+  T *operator->();
+};
+
+struct Struct {
+  int len;
+};
+
+template 
+struct TemplateStruct {
+  Ptr val();  // expected-note {{declared here}}
+};
+
+template 
+void templateFunc(const TemplateStruct ) {
+  Ptr ptr = ts.val(); // expected-error {{function is not marked const}}
+  auto foo = ptr->len;
+}
+
+template void templateFunc<0>(const TemplateStruct<0> &);  // expected-note {{requested here}}
+
+} // namespace clangd_issue_1073_no_crash_dependent_type
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14705,6 +14705,8 @@
   return getSema().CreateBuiltinArraySubscriptExpr(
   First, Callee->getBeginLoc(), Second, OpLoc);
   } else if (Op == OO_Arrow) {
+if (First->getType()->isDependentType())
+  return ExprError();
 // -> is never a builtin operation.
 return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I reduced something very similar recently as 
https://github.com/clangd/clangd/issues/1073

This patch does not fix it, but looks closely related, want to take a look?


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

[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.
This revision is now accepted and ready to land.

Thanks, looks good!

please update the description of the patch accordingly (saying we invalidate 
the vardecl)




Comment at: clang/test/SemaCXX/arrow-operator.cpp:79
+void foo() {
+  // x is dependent.
+  A& x = blah[7];  // expected-error {{use of undeclared identifier 
'blah'}}

I think the comment `x is dependent` should associate to `x->call()` statement 
nor the vardecl.


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

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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added a comment.

Thanks!

I updated the change. Let me know if this is what you had in mind.
I kept the original test too, can't hurt right?


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

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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz updated this revision to Diff 416561.
adamcz added a comment.

changed to marking VarDecl as invalid if it's ref type and initializer is 
invalid


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121824/new/

https://reviews.llvm.org/D121824

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/AST/ast-dump-invalid-initialized.cpp
  clang/test/SemaCXX/arrow-operator.cpp


Index: clang/test/SemaCXX/arrow-operator.cpp
===
--- clang/test/SemaCXX/arrow-operator.cpp
+++ clang/test/SemaCXX/arrow-operator.cpp
@@ -65,3 +65,24 @@
 }
 
 } // namespace arrow_suggest
+
+namespace no_crash_dependent_type {
+
+template
+struct A {
+  void call();
+  A* operator->();
+};
+
+template 
+void foo() {
+  // x is dependent.
+  A& x = blah[7];  // expected-error {{use of undeclared identifier 
'blah'}}
+  x->call();
+}
+
+void test() {
+  foo();
+}
+
+} // namespace no_crash_dependent_type
Index: clang/test/AST/ast-dump-invalid-initialized.cpp
===
--- clang/test/AST/ast-dump-invalid-initialized.cpp
+++ clang/test/AST/ast-dump-invalid-initialized.cpp
@@ -24,4 +24,6 @@
   auto b4 = A(1);
   // CHECK: `-VarDecl {{.*}} invalid b5 'auto'
   auto b5 = A{1};
-}
\ No newline at end of file
+  // CHECK: `-VarDecl {{.*}} invalid b6 'A &'
+  A& b6 = garbage[1];
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12794,6 +12794,12 @@
 return;
   }
 
+  // Reference type without initializer doesn't work.
+  if (VD->getType()->isReferenceType()) {
+VD->setInvalidDecl();
+return;
+  }
+
   QualType Ty = VD->getType();
   if (Ty->isDependentType()) return;
 


Index: clang/test/SemaCXX/arrow-operator.cpp
===
--- clang/test/SemaCXX/arrow-operator.cpp
+++ clang/test/SemaCXX/arrow-operator.cpp
@@ -65,3 +65,24 @@
 }
 
 } // namespace arrow_suggest
+
+namespace no_crash_dependent_type {
+
+template
+struct A {
+  void call();
+  A* operator->();
+};
+
+template 
+void foo() {
+  // x is dependent.
+  A& x = blah[7];  // expected-error {{use of undeclared identifier 'blah'}}
+  x->call();
+}
+
+void test() {
+  foo();
+}
+
+} // namespace no_crash_dependent_type
Index: clang/test/AST/ast-dump-invalid-initialized.cpp
===
--- clang/test/AST/ast-dump-invalid-initialized.cpp
+++ clang/test/AST/ast-dump-invalid-initialized.cpp
@@ -24,4 +24,6 @@
   auto b4 = A(1);
   // CHECK: `-VarDecl {{.*}} invalid b5 'auto'
   auto b5 = A{1};
-}
\ No newline at end of file
+  // CHECK: `-VarDecl {{.*}} invalid b6 'A &'
+  A& b6 = garbage[1];
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -12794,6 +12794,12 @@
 return;
   }
 
+  // Reference type without initializer doesn't work.
+  if (VD->getType()->isReferenceType()) {
+VD->setInvalidDecl();
+return;
+  }
+
   QualType Ty = VD->getType();
   if (Ty->isDependentType()) return;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for looking at this. It seems an issue caused by 
https://reviews.llvm.org/D120812 (sorry), where we build a recovery-expr to 
model declrefexpr pointing to an invalid decl.




Comment at: clang/lib/Sema/TreeTransform.h:14709
 // -> is never a builtin operation.
+if (First->getType()->isDependentType())
+  return ExprError();

It looks like we are somehow doing a transformation on a dependent-type 
expression (I think it is the recoveryExpression) during the template 
instantiation, which leads to the crash.




Comment at: clang/test/SemaCXX/arrow-operator.cpp:79
+void foo() {
+  // x is dependent.
+  A& x = blah[7];  // expected-error {{use of undeclared identifier 
'blah'}} \

The AST nodes looks weird and inconsistent in the primary template and 
instantiated template.

VarDecl `x` AST node in the primary template look like (with a Valid bit!)

```
`-DeclStmt 
   `-VarDecl  x 'A &'
```

while in the instantiated template (with an **Invalid** bit!):
```
 `-DeclStmt
`-VarDecl invalid x 'A &'
```

The difference of valid bit results in two different forms of the expression 
`x->call()`  (a normal CXXMemberCallExpr vs. a dependent-type CallExpr wrapping 
a RecoveryExpr), I think this is likely the root cause of the crash.

If we invalidate the VarDecl x in the primary template for this case, the issue 
should be gone. This seems a reasonable fix to me -- a reference-type VarDecl 
is invalid, when it doesn't have an initializer (either 1. it is not written in 
the source code or 2. it is too malformed to preserve ).  Clang AST already 
does 1.  We're missing 2, I think it is in `Sema::ActOnInitializerError`.





Comment at: clang/test/SemaCXX/arrow-operator.cpp:81
+  A& x = blah[7];  // expected-error {{use of undeclared identifier 
'blah'}} \
+// expected-error {{declaration of reference variable 
'x' requires an initializer}}
+  x->call();

if we mark vardecl x invalid, I think the bogus `requires an initializer` 
diagnostic will be gone.


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

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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz created this revision.
adamcz added a reviewer: hokein.
Herald added a project: All.
adamcz requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121824

Files:
  clang/lib/Sema/TreeTransform.h
  clang/test/SemaCXX/arrow-operator.cpp


Index: clang/test/SemaCXX/arrow-operator.cpp
===
--- clang/test/SemaCXX/arrow-operator.cpp
+++ clang/test/SemaCXX/arrow-operator.cpp
@@ -65,3 +65,25 @@
 }
 
 } // namespace arrow_suggest
+
+namespace no_crash_dependent_type {
+
+template
+struct A {
+  void call();
+  A* operator->();
+};
+
+template 
+void foo() {
+  // x is dependent.
+  A& x = blah[7];  // expected-error {{use of undeclared identifier 
'blah'}} \
+// expected-error {{declaration of reference variable 
'x' requires an initializer}}
+  x->call();
+}
+
+void test() {
+  foo();  // expected-note {{in instantiation}}
+}
+
+} // namespace no_crash_dependent_type
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14706,6 +14706,8 @@
   First, Callee->getBeginLoc(), Second, OpLoc);
   } else if (Op == OO_Arrow) {
 // -> is never a builtin operation.
+if (First->getType()->isDependentType())
+  return ExprError();
 return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {
 if (!First->getType()->isOverloadableType() ||


Index: clang/test/SemaCXX/arrow-operator.cpp
===
--- clang/test/SemaCXX/arrow-operator.cpp
+++ clang/test/SemaCXX/arrow-operator.cpp
@@ -65,3 +65,25 @@
 }
 
 } // namespace arrow_suggest
+
+namespace no_crash_dependent_type {
+
+template
+struct A {
+  void call();
+  A* operator->();
+};
+
+template 
+void foo() {
+  // x is dependent.
+  A& x = blah[7];  // expected-error {{use of undeclared identifier 'blah'}} \
+// expected-error {{declaration of reference variable 'x' requires an initializer}}
+  x->call();
+}
+
+void test() {
+  foo();  // expected-note {{in instantiation}}
+}
+
+} // namespace no_crash_dependent_type
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -14706,6 +14706,8 @@
   First, Callee->getBeginLoc(), Second, OpLoc);
   } else if (Op == OO_Arrow) {
 // -> is never a builtin operation.
+if (First->getType()->isDependentType())
+  return ExprError();
 return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
   } else if (Second == nullptr || isPostIncDec) {
 if (!First->getType()->isOverloadableType() ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits