[clang][HeuristicResolver] Track the expression whose type is being simplified after each step in simplifyType()

[HighCommander4]

Fixes #126536

[llvmbot]

@llvm/pr-subscribers-clang

Author: Nathan Ridge (HighCommander4)

Changes

Fixes #126536

---

Full diff: https://github.com/llvm/llvm-project/pull/126689.diff

2 Files Affected:

• (modified) clang/lib/Sema/HeuristicResolver.cpp (+29-18)
• (modified) clang/unittests/Sema/HeuristicResolverTest.cpp (+16)

diff --git a/clang/lib/Sema/HeuristicResolver.cpp b/clang/lib/Sema/HeuristicResolver.cpp
index 36e5b44b8b12cc1..3cbf33dcdced383 100644
--- a/clang/lib/Sema/HeuristicResolver.cpp
+++ b/clang/lib/Sema/HeuristicResolver.cpp
@@ -210,52 +210,63 @@ QualType HeuristicResolverImpl::getPointeeType(QualType T) {
 QualType HeuristicResolverImpl::simplifyType(QualType Type, const Expr *E,
                                              bool UnwrapPointer) {
   bool DidUnwrapPointer = false;
-  auto SimplifyOneStep = [&](QualType T) {
+  // A type, together with an optional expression whose type it represents
+  // which may have additional information about the expression's type
+  // not stored in the QualType itself.
+  struct TypeExprPair {
+    QualType Type;
+    const Expr *E = nullptr;
+  };
+  TypeExprPair Current{Type, E};
+  auto SimplifyOneStep = [UnwrapPointer, &DidUnwrapPointer,
+                          this](TypeExprPair T) -> TypeExprPair {
     if (UnwrapPointer) {
-      if (QualType Pointee = getPointeeType(T); !Pointee.isNull()) {
+      if (QualType Pointee = getPointeeType(T.Type); !Pointee.isNull()) {
         DidUnwrapPointer = true;
-        return Pointee;
+        return {Pointee};
       }
     }
-    if (const auto *RT = T->getAs<ReferenceType>()) {
+    if (const auto *RT = T.Type->getAs<ReferenceType>()) {
       // Does not count as "unwrap pointer".
-      return RT->getPointeeType();
+      return {RT->getPointeeType()};
     }
-    if (const auto *BT = T->getAs<BuiltinType>()) {
+    if (const auto *BT = T.Type->getAs<BuiltinType>()) {
       // If BaseType is the type of a dependent expression, it's just
       // represented as BuiltinType::Dependent which gives us no information. We
       // can get further by analyzing the dependent expression.
-      if (E && BT->getKind() == BuiltinType::Dependent) {
-        return resolveExprToType(E);
+      if (T.E && BT->getKind() == BuiltinType::Dependent) {
+        return {resolveExprToType(T.E), T.E};
       }
     }
-    if (const auto *AT = T->getContainedAutoType()) {
+    if (const auto *AT = T.Type->getContainedAutoType()) {
       // If T contains a dependent `auto` type, deduction will not have
       // been performed on it yet. In simple cases (e.g. `auto` variable with
       // initializer), get the approximate type that would result from
       // deduction.
       // FIXME: A more accurate implementation would propagate things like the
       // `const` in `const auto`.
-      if (E && AT->isUndeducedAutoType()) {
-        if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+      if (T.E && AT->isUndeducedAutoType()) {
+        if (const auto *DRE = dyn_cast<DeclRefExpr>(T.E)) {
           if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
-            if (VD->hasInit())
-              return resolveExprToType(VD->getInit());
+            if (VD->hasInit()) {
+              auto *Init = VD->getInit();
+              return {resolveExprToType(Init), Init};
+            }
           }
         }
       }
     }
     return T;
   };
-  while (!Type.isNull()) {
-    QualType New = SimplifyOneStep(Type);
-    if (New == Type)
+  while (!Current.Type.isNull()) {
+    TypeExprPair New = SimplifyOneStep(Current);
+    if (New.Type == Current.Type)
       break;
-    Type = New;
+    Current = New;
   }
   if (UnwrapPointer && !DidUnwrapPointer)
     return QualType();
-  return Type;
+  return Current.Type;
 }
 
 std::vector<const NamedDecl *> HeuristicResolverImpl::resolveMemberExpr(
diff --git a/clang/unittests/Sema/HeuristicResolverTest.cpp b/clang/unittests/Sema/HeuristicResolverTest.cpp
index 5c3459dbeb1018b..c7cfe7917c532e6 100644
--- a/clang/unittests/Sema/HeuristicResolverTest.cpp
+++ b/clang/unittests/Sema/HeuristicResolverTest.cpp
@@ -394,6 +394,22 @@ TEST(HeuristicResolver, MemberExpr_DeducedNonTypeTemplateParameter) {
       fieldDecl(hasName("found")).bind("output"));
 }
 
+TEST(HeuristicResolver, MemberExpr_HangIssue126536) {
+  std::string Code = R"cpp(
+    template <class T>
+    void foo() {
+      T bar;
+      auto baz = (bar, bar);
+      baz.foo();
+    }
+  )cpp";
+  // Test resolution of "foo" in "baz.foo()".
+  // Here, we are testing that we do not get into an infinite loop.
+  expectResolution(
+      Code, &HeuristicResolver::resolveMemberExpr,
+      cxxDependentScopeMemberExpr(hasMemberName("foo")).bind("input"));
+}
+
 TEST(HeuristicResolver, DeclRefExpr_StaticMethod) {
   std::string Code = R"cpp(
     template <typename T>

[HighCommander4]

The explanation in this comment may provide useful context for reviewing this fix.

[zyn0217]

Thanks for the prompt fix. The explanation makes sense to me, however please wait for @kadircet to confirm this fixes their problems.

(As an aside, can you flesh out the PR's description body before you commit? It would help others to reason out the patch)

[kadircet]

thanks, lgtm! taking a closer look here, I feel more inclined towards having the step limit in the release branch. I think because we're resolving the initializer, we can still end up in situations where we have equivalent (T,E) pairs, even if not the same.

[HighCommander4]

(Rebased)

[llvm-ci]

LLVM Buildbot has detected a new failure on builder openmp-s390x-linux running on systemz-1 while building clang at step 6 "test-openmp".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/7907

Here is the relevant piece of the build log for the reference

Step 6 (test-openmp) failure: 1200 seconds without output running [b'ninja', b'-j 4', b'check-openmp'], attempting to kill
...
PASS: ompd-test :: openmp_examples/example_3.c (439 of 449)
PASS: ompd-test :: openmp_examples/example_4.c (440 of 449)
PASS: ompd-test :: openmp_examples/example_5.c (441 of 449)
PASS: ompd-test :: openmp_examples/example_task.c (442 of 449)
UNSUPPORTED: ompd-test :: openmp_examples/ompd_bt.c (443 of 449)
PASS: ompd-test :: openmp_examples/fibonacci.c (444 of 449)
UNSUPPORTED: ompd-test :: openmp_examples/ompd_parallel.c (445 of 449)
PASS: ompd-test :: openmp_examples/parallel.c (446 of 449)
PASS: ompd-test :: openmp_examples/nested.c (447 of 449)
PASS: ompd-test :: openmp_examples/ompd_icvs.c (448 of 449)
command timed out: 1200 seconds without output running [b'ninja', b'-j 4', b'check-openmp'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1316.335579