[PATCH] D78760: Check a class has a definition before iterating over its base classes

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D78760#2005874 , @ahatanak wrote:

> `TagDecl::isDependentType()` is returning true.


Oh, sorry, we do already do the thing I was suggesting. But in your testcase 
the issue is that the base class `B0` is dependent but is also resolved during 
template instantiation to a (dependent) class type. We should handle this by 
skipping dependent base classes, not by skipping incomplete base classes. For 
example, consider this testcase (which currently asserts):

  struct A {};
  template struct X {
struct B : A {};
struct C : A, B {};
  };

here, we can resolve the `B` base of `C` to the member `C` of the current 
instantiation, but we should not traverse to `B`'s base class list when 
considering the base classes of `C`, because we do not know that that 
definition of `B` will be used by any particular instantiation of `C`. For 
example:

  template<> struct X::B {};
  X::C c; // has only one `A` base class


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78760



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78760: Check a class has a definition before iterating over its base classes

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D78760#2003118 , @rsmith wrote:

> If so, we'll need to make sure that all code that iterates over base classes 
> checks for this condition (I bet there are more cases than the two that you 
> found).


Just a potentially related thought: I was recently trying to understand under 
what circumstances should I expect `CXXBaseSpecifier::getType()` to return 
`QualType` instance for which `isNull()` could be `true`. I gave up and don't 
make any assumption.
Would the suggested change affect this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78760



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78760: Check a class has a definition before iterating over its base classes

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

`TagDecl::isDependentType()` is returning true.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78760



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78760: Check a class has a definition before iterating over its base classes

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

I think the non-dependent base class you mention in this case is `B0`, but it 
seems that the type of `B0` in the AST is dependent as `isDependentType` 
returns true. Should we fix this? Or does `isDependentType` mean something 
different?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78760



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78760: Check a class has a definition before iterating over its base classes

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

We should reject a definition of a class that has a non-dependent base class 
that's an incomplete type. We need non-dependent bases to be complete in order 
to be able to do name lookup into them when processing the template definition. 
(The C++ standard permits us to reject such cases but does not require it, 
because "a hypothetical instantiation of [the templated class] immediately 
following its definition would be ill-formed due to a construct that does not 
depend on a template parameter".)

Then there's a question of error recovery and AST invariants: should we permit 
a base specifier to name a non-dependent type that's not a complete class type? 
If so, we'll need to make sure that all code that iterates over base classes 
checks for this condition (I bet there are more cases than the two that you 
found). But tooling use cases probably do want to know that such base 
specifiers were written. So perhaps we should allow such base classes but mark 
the class definition as invalid if we find them -- in which case we would want 
something like this patch as part of the error recovery.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78760



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78760: Check a class has a definition before iterating over its base classes

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

I'm not sure why this shouldn't be caught in `Sema::CheckBaseSpecifier`. But 
there is a check for the definition of the class before 
`findCircularInheritance` is called, so I'm guessing there is a reason the code 
shouldn't be rejected here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78760



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78760: Check a class has a definition before iterating over its base classes

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak created this revision.
ahatanak added a reviewer: rsmith.
ahatanak added a project: clang.
Herald added subscribers: ributzka, dexonsmith, jkorous.
ahatanak added a comment.

I'm not sure why this shouldn't be caught in `Sema::CheckBaseSpecifier`. But 
there is a check for the definition of the class before 
`findCircularInheritance` is called, so I'm guessing there is a reason the code 
shouldn't be rejected here.


This fixes a crash when a class defined in a method of a templated class 
inherits from a class that is forward-declared.

I considered checking whether the base class is defined in 
`Sema::CheckBaseSpecifier` and rejecting the code if it isn't, but it seems 
like that's not how this should be fixed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78760

Files:
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/class.cpp


Index: clang/test/SemaCXX/class.cpp
===
--- clang/test/SemaCXX/class.cpp
+++ clang/test/SemaCXX/class.cpp
@@ -211,3 +211,21 @@
 struct PR9989 { 
   static int const PR9989_Member = sizeof PR9989_Member; 
 };
+
+namespace forward_declaration {
+struct B1 {};
+
+template 
+struct S0 {
+  void m0() {
+struct B0; // expected-note {{member is declared here}}
+
+struct D : B0, B1 { // expected-error {{implicit instantiation}} 
expected-note {{in instantiation of}}
+};
+  }
+};
+
+void test() {
+  S0().m0(); // expected-note {{in instantiation of}}
+}
+} // namespace forward_declaration
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -2611,6 +2611,9 @@
   if (auto Rec = Type->getAs()) {
 auto Decl = Rec->getAsCXXRecordDecl();
 
+if (!Decl->hasDefinition())
+  return;
+
 // Iterate over its bases.
 for (const auto  : Decl->bases()) {
   QualType Base = Context.getCanonicalType(BaseSpec.getType())
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -173,6 +173,8 @@
   SmallVector WorkList = {StartRD};
   while (!WorkList.empty()) {
 const CXXRecordDecl *RD = WorkList.pop_back_val();
+if (!RD->hasDefinition())
+  continue;
 for (const CXXBaseSpecifier  : RD->bases()) {
   if (const CXXRecordDecl *B = BaseSpec.getType()->getAsCXXRecordDecl()) {
 if (!SeenBaseTypes.insert(B).second)


Index: clang/test/SemaCXX/class.cpp
===
--- clang/test/SemaCXX/class.cpp
+++ clang/test/SemaCXX/class.cpp
@@ -211,3 +211,21 @@
 struct PR9989 { 
   static int const PR9989_Member = sizeof PR9989_Member; 
 };
+
+namespace forward_declaration {
+struct B1 {};
+
+template 
+struct S0 {
+  void m0() {
+struct B0; // expected-note {{member is declared here}}
+
+struct D : B0, B1 { // expected-error {{implicit instantiation}} expected-note {{in instantiation of}}
+};
+  }
+};
+
+void test() {
+  S0().m0(); // expected-note {{in instantiation of}}
+}
+} // namespace forward_declaration
Index: clang/lib/Sema/SemaDeclCXX.cpp
===
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -2611,6 +2611,9 @@
   if (auto Rec = Type->getAs()) {
 auto Decl = Rec->getAsCXXRecordDecl();
 
+if (!Decl->hasDefinition())
+  return;
+
 // Iterate over its bases.
 for (const auto  : Decl->bases()) {
   QualType Base = Context.getCanonicalType(BaseSpec.getType())
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -173,6 +173,8 @@
   SmallVector WorkList = {StartRD};
   while (!WorkList.empty()) {
 const CXXRecordDecl *RD = WorkList.pop_back_val();
+if (!RD->hasDefinition())
+  continue;
 for (const CXXBaseSpecifier  : RD->bases()) {
   if (const CXXRecordDecl *B = BaseSpec.getType()->getAsCXXRecordDecl()) {
 if (!SeenBaseTypes.insert(B).second)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits