[clang] Add the check of membership for the issue #58674 and improve the lookup process

[lime]

Summary

This patch includes the commit 01adf96ebc86 and a fix of unhandled declaration
references.

When looking up base classes, Clang first checks whether a base class is a
template and takes the specialized template based on it. However, the base class
might be instantiated, and the above behavior can lose information.

This patch fixes the problem by first checking whether a base class is a record
declaration, so the instantiated one will be taken.

[lime]

clang/lib/AST/CXXInheritance.cpp

85

BTW, I forgot to revert this condition last time.

[lime]

Ping!

[cor3ntin]

Thanks for working on this issue.
I don't feel confident commenting on this patch's approach (I think @erichkeane is your best bet), but the small typo does make a pretty big difference here!

clang/include/clang/AST/DeclCXX.h

1549

1553

Ditto in the rest of the patch

[erichkeane]

I don't have a great idea of this section of code, but as code-owner I guess I'm stuck approving/disapproving of this :) Barring additional information, I think I have to accept this approach, it seems to work right, and from some time debugging, it at least doesn't seem like the 'wrong' answer here. I think the suggestion corentin made is correct and needs to be made, but otherwise I consider this acceptable.

clang/include/clang/AST/DeclCXX.h

1553

I suggest making this just LookupInDependentTypes to be more clear.

[lime]

Updated as suggested. I will do a stage-2 build before landing the patch to confirm whether it would case another crash, and it takes time.

[joanahalili]

Heads up: We are experiencing a series of clang crashes because of this commit.

What we already have so far:

Unhandled DeclRefExpr
UNREACHABLE executed at clang/lib/CodeGen/CGExpr.cpp:2958!

We are now working on a reproducer.

[joanahalili]

In D143840#4147234, @joanahalili wrote:

Heads up: We are experiencing a series of clang crashes because of this commit.

What we already have so far:

Unhandled DeclRefExpr
UNREACHABLE executed at clang/lib/CodeGen/CGExpr.cpp:2958!

We are now working on a reproducer.

Here is our reproducer:

struct a {
  enum { b };
};
struct c {
  void *d;
};
struct e {
  void f(void *);
};
template <typename g> struct h : g {
  h(short, int);
  virtual void i();
};
template <typename g> void h<g>::i() { e().f(c::d); }
struct j : h<c> {
  j();
};
int k;
j::j() : h(a::b, k) {}

[alexfh]

In D143840#4147582, @joanahalili wrote:

In D143840#4147234, @joanahalili wrote:

Heads up: We are experiencing a series of clang crashes because of this commit.

What we already have so far:

Unhandled DeclRefExpr
UNREACHABLE executed at clang/lib/CodeGen/CGExpr.cpp:2958!

We are now working on a reproducer.

Here is our reproducer:

struct a {
  enum { b };
};
struct c {
  void *d;
};
struct e {
  void f(void *);
};
template <typename g> struct h : g {
  h(short, int);
  virtual void i();
};
template <typename g> void h<g>::i() { e().f(c::d); }
struct j : h<c> {
  j();
};
int k;
j::j() : h(a::b, k) {}

Thanks! This looks like a real problem. I'm going to revert the commit.

BTW, I managed to reduce your test case slightly more:

struct f {
  void *g;
};
template <typename j> struct k : j {
  virtual void l(){ (void)f::g; }
};
k<f> q;

https://gcc.godbolt.org/z/fa33GYf9K

[rupprecht]

This has already been reverted, but I found a breakage (not a crash) caused by this:

#include <type_traits>

class Base {
 protected:
  int member;
};

template <typename Parent>
struct Subclass : public Parent {
  static_assert(std::is_base_of<Base, Parent>::value,
                "Parent not derived from Base");
  int func() { return Base::member; }
};

using Impl = Subclass<Base>;

int use() {
  Impl i;
  return i.func();
}

gcc/msvc both accept this. But clang says:

<source>:8:29: error: 'member' is a protected member of 'Base'
  int func() { return Base::member; }
                            ^
<source>:15:12: note: in instantiation of member function 'Subclass<Base>::func' requested here
  return i.func();
           ^
<source>:3:7: note: must name member using the type of the current context 'Subclass<Base>'
  int member;
      ^

A fix is to write Parent::member instead of Base::member, but I'm wondering what the spec actually says about this, and if clang is right to reject it.

clang/docs/ReleaseNotes.rst

 ^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Fix crash on invalid code when looking up a destructor in a templated class
   inside a namespace.
   (`#59446 <https://github.com/llvm/llvm-project/issues/59446>`_)
 - Fix crash when evaluating consteval constructor of derived class whose base
   has more than one field.
   (`#60166 <https://github.com/llvm/llvm-project/issues/60166>`_)
+- Fix an issue about ``decltype`` in the members of class templates derived from
+  templates with related parameters.
+  (`#58674 <https://github.com/llvm/llvm-project/issues/58674>`_)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Bug Fixes
 ^^^^^^^^^^^^^^^^^^^^^^^
 
 Miscellaneous Clang Crashes Fixed

clang/include/clang/AST/DeclCXX.h

   ///
   /// This routine only determines whether this class is derived from \p Base,
   /// but does not account for factors that may make a Derived -> Base class
   /// ill-formed, such as private/protected inheritance or multiple, ambiguous
   /// base class subobjects.
   ///
   /// \param Base the base class we are searching for.
   ///
+  /// \param LookupInDependentTypes whether to look up in dependent types.
+  ///
   /// \returns true if this class is derived from Base, false otherwise.
-  bool isDerivedFrom(const CXXRecordDecl *Base) const;
+  bool isDerivedFrom(const CXXRecordDecl *Base,
+                     bool LookupInDependentTypes = false) const;
 
   /// Determine whether this class is derived from the type \p Base.
   ///
   /// This routine only determines whether this class is derived from \p Base,
   /// but does not account for factors that may make a Derived -> Base class
   /// ill-formed, such as private/protected inheritance or multiple, ambiguous
   /// base class subobjects.
   ///
   /// \param Base the base class we are searching for.
   ///
   /// \param Paths will contain the paths taken from the current class to the
   /// given \p Base class.
   ///
+  /// \param LookupInDependentTypes whether to look up independent types.
+  ///
   /// \returns true if this class is derived from \p Base, false otherwise.
   ///
   /// \todo add a separate parameter to configure IsDerivedFrom, rather than
   /// tangling input and output in \p Paths
-  bool isDerivedFrom(const CXXRecordDecl *Base, CXXBasePaths &Paths) const;
+  bool isDerivedFrom(const CXXRecordDecl *Base, CXXBasePaths &Paths,
+                     bool LookupInDependentTypes = false) const;
 
   /// Determine whether this class is virtually derived from
   /// the class \p Base.
   ///
   /// This routine only determines whether this class is virtually
   /// derived from \p Base, but does not account for factors that may
   /// make a Derived -> Base class ill-formed, such as
   /// private/protected inheritance or multiple, ambiguous base class

clang/lib/AST/CXXInheritance.cpp

void CXXBasePaths::swap(CXXBasePaths &Other) {
   ClassSubobjects.swap(Other.ClassSubobjects);
   VisitedDependentRecords.swap(Other.VisitedDependentRecords);
   std::swap(FindAmbiguities, Other.FindAmbiguities);
   std::swap(RecordPaths, Other.RecordPaths);
   std::swap(DetectVirtual, Other.DetectVirtual);
   std::swap(DetectedVirtual, Other.DetectedVirtual);
 }
 
-bool CXXRecordDecl::isDerivedFrom(const CXXRecordDecl *Base) const {
+bool CXXRecordDecl::isDerivedFrom(const CXXRecordDecl *Base,
+                                  bool LookupInDependentTypes) const {
   CXXBasePaths Paths(/*FindAmbiguities=*/false, /*RecordPaths=*/false,
                      /*DetectVirtual=*/false);
-  return isDerivedFrom(Base, Paths);
+  return isDerivedFrom(Base, Paths, LookupInDependentTypes);
 }
 
 bool CXXRecordDecl::isDerivedFrom(const CXXRecordDecl *Base,
-                                  CXXBasePaths &Paths) const {
+                                  CXXBasePaths &Paths,
+                                  bool LookupInDependentTypes) const {
   if (getCanonicalDecl() == Base->getCanonicalDecl())
     return false;
 
   Paths.setOrigin(const_cast<CXXRecordDecl*>(this));
 
   const CXXRecordDecl *BaseDecl = Base->getCanonicalDecl();
   return lookupInBases(
       [BaseDecl](const CXXBaseSpecifier *Specifier, CXXBasePath &Path) {
         return Specifier->getType()->getAsRecordDecl() &&
                FindBaseClass(Specifier, Path, BaseDecl);
       },
-      Paths);
+      Paths, LookupInDependentTypes);
 }
 
 bool CXXRecordDecl::isVirtuallyDerivedFrom(const CXXRecordDecl *Base) const {
   if (!getNumVBases())
     return false;
 
   CXXBasePaths Paths(/*FindAmbiguities=*/false, /*RecordPaths=*/false,
                      /*DetectVirtual=*/false);
if (BaseMatches(&BaseSpec, ScratchPath)) {
       } else if (!isFindingAmbiguities()) {
         // We found a path and we don't care about ambiguities;
         // return immediately.
         return FoundPath;
       }
     } else if (VisitBase) {
       CXXRecordDecl *BaseRecord;
       if (LookupInDependent) {
-        BaseRecord = nullptr;
-        const TemplateSpecializationType *TST =
-            BaseSpec.getType()->getAs<TemplateSpecializationType>();
-        if (!TST) {
-          if (auto *RT = BaseSpec.getType()->getAs<RecordType>())
-            BaseRecord = cast<CXXRecordDecl>(RT->getDecl());
-        } else {
+        BaseRecord = cast_if_present<CXXRecordDecl>(
+            BaseSpec.getType()->getAsRecordDecl());
+        if (!BaseRecord) {
+          if (const TemplateSpecializationType *TST =
+                  BaseSpec.getType()->getAs<TemplateSpecializationType>()) {
             TemplateName TN = TST->getTemplateName();
             if (auto *TD =
                     dyn_cast_or_null<ClassTemplateDecl>(TN.getAsTemplateDecl()))
               BaseRecord = TD->getTemplatedDecl();
           }
+        }
         if (BaseRecord) {
           if (!BaseRecord->hasDefinition() ||
               VisitedDependentRecords.count(BaseRecord)) {
             BaseRecord = nullptr;
           } else {
             VisitedDependentRecords.insert(BaseRecord);
           }
         }

clang/lib/Sema/SemaExpr.cpp

Sema::ActOnIdExpression(Scope *S, CXXScopeSpec &SS,
   //   Otherwise, if E1.E2 refers to a non-static member function. . .
   //   [t]he expression can be used only as the left-hand operand of a
   //   member function call.
   //
   // There are other safeguards against such uses, but it's important
   // to get this right here so that we don't end up making a
   // spuriously dependent expression if we're inside a dependent
   // instance method.
+  //
+  // We also don't need to do this if R resolved to a member in another
+  // class, which can happen in an unevaluated operand:
+  //
+  // C++ [expr.prim.id]p3.3:
+  //   If that id-expression denotes a non-static data member and it
+  //   appears in an unevaluated operand.
   if (!R.empty() && (*R.begin())->isCXXClassMember()) {
-    bool MightBeImplicitMember;
-    if (!IsAddressOfOperand)
-      MightBeImplicitMember = true;
-    else if (!SS.isEmpty())
-      MightBeImplicitMember = false;
-    else if (R.isOverloadedResult())
+    bool MightBeImplicitMember = true, CheckField = true;
+    if (IsAddressOfOperand) {
+      MightBeImplicitMember = SS.isEmpty() && !R.isOverloadedResult();
+      CheckField = !R.isUnresolvableResult();
+    }
+    if (MightBeImplicitMember && CheckField) {
+      if (R.isSingleResult() &&
+          isa<FieldDecl, IndirectFieldDecl, MSPropertyDecl>(R.getFoundDecl())) {
+        auto Class = cast<CXXRecordDecl>((*R.begin())->getDeclContext());
+        for (auto Curr = S->getLookupEntity(); Curr && !Curr->isFileContext();
+             Curr = Curr->getParent()) {
+          if (auto ThisClass = dyn_cast_if_present<CXXRecordDecl>(Curr)) {
+            if ((MightBeImplicitMember =
+                     ThisClass->Equals(Class) ||
+                     ThisClass->isDerivedFrom(Class,
+                                              /*LookupIndependent=*/true)))
+              break;
+          }
+        }
+      } else if (IsAddressOfOperand)
         MightBeImplicitMember = false;
-    else if (R.isUnresolvableResult())
-      MightBeImplicitMember = true;
-    else
-      MightBeImplicitMember = isa<FieldDecl>(R.getFoundDecl()) ||
-                              isa<IndirectFieldDecl>(R.getFoundDecl()) ||
-                              isa<MSPropertyDecl>(R.getFoundDecl());
+    }
 
     if (MightBeImplicitMember)
       return BuildPossibleImplicitMemberExpr(SS, TemplateKWLoc,
                                              R, TemplateArgs, S);
   }
 
   if (TemplateArgs || TemplateKWLoc.isValid()) {
 

clang/test/CodeGenCXX/decl-ref-inheritance.cpp

+// RUN: %clang_cc1 -triple=x86_64-unknown-linux -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: [[FOO:%.+]] = type { i32 }
+struct foo {
+  int val;
+};
+
+template <typename T> struct bar : T {
+};
+
+struct baz : bar<foo> {
+  // CHECK-LABEL: define{{.*}} i32 @_ZN3baz3getEv
+  // CHECK: {{%.+}} = getelementptr inbounds [[FOO]], ptr {{%.+}}, i32 0, i32 0
+  int get() {
+    return val;
+  }
+};
+
+int qux() {
+  auto f = baz{};
+  return f.get();
+}

clang/test/SemaCXX/decltype.cpp

namespace D5789 {
   void foo(decltype(T(LP1{ .p1 = { "foo" }, .p1.x[1] = 'x'}))) {} // okay
 
   // expected-warning@+3 {{initializer partially overrides}}
   // expected-note@+2 {{previous initialization}}
   // expected-error@+1 {{redefinition of 'foo'}}
   template<class T> void foo(decltype(T(LP1{ .p1 = g1, .p1.x[1] = 'x' }))) {}
 }
 
+namespace GH58674 {
+  struct Foo {
+    float value_;
+    struct nested {
+      float value_;
+    };
+  };
+
+  template <typename T>
+  struct TemplateFoo {
+    float value_;
+  };
+
+  float bar;
+
+  template <typename T>
+  struct Animal{};
+
+  template <typename T>
+  class Cat : Animal<T> {
+    using okay = decltype(Foo::value_);
+    using also_okay = decltype(bar);
+    using okay2 = decltype(Foo::nested::value_);
+    using okay3 = decltype(TemplateFoo<T>::value_);
+  public:
+    void meow() {
+      using okay = decltype(Foo::value_);
+      using also_okay = decltype(bar);
+      using okay2 = decltype(Foo::nested::value_);
+      using okay3 = decltype(TemplateFoo<T>::value_);
+    }
+  };
+
+  void baz() {
+      Cat<void>{}.meow();
+  }
+}
+
 template<typename>
 class conditional {
 };
 
 // FIXME: The diagnostics here are produced twice.
 void foo(conditional<decltype((1),int>) {  // expected-note 2 {{to match this '('}} expected-error {{expected ')'}} expected-note 2{{to match this '<'}}
 } // expected-error {{expected function body after function declarator}} expected-error 2 {{expected '>'}} expected-error {{expected ')'}}