[clang] Fix non-deterministic infinite recursion... #118288

alejandro-alvarez-sonarsource · 2024-12-02T12:57:38Z

...in ASTContext::getAutoTypeInternal

Given

template < typename >
concept C1 = true;

template < typename , auto >
concept C2 = true;

template < C1 auto V, C2< V > auto>
struct S;

Both C1 auto V and C2<V> auto end on the set AutoType, the former being a template parameter for the latter.

Since the hashing is not deterministic (i.e., pointers are hashed), every now and then, both will end on the same bucket. Given that FoldingSet recomputes the FoldingSetID for each node in the target bucket on lookup, this triggers an infinite recursion:

Look for X in AutoTypes
Let's assume it would be in bucket N, so it iterates over nodes in that bucket. Let's assume the first is C2<V> auto.
Computes the FoldingSetID for this one, which requires the profile of its template parameters, so they are visited.
In some frames below, we end on the same FoldingSet, and, by chance, C1 auto V would be in bucket N too.
But the first node in the bucket is C2<V> auto for which we need to profile C1 auto V
... stack overflow!

No step individually does anything wrong, but in general, FoldingSet seems not to be re-entrant, and this fact is hidden behind many nested calls.

With this change, we store the AutoTypes inside a DenseMap instead. The FoldingSetID is computed once only and then kept as the map's key, avoiding the need to do recursive lookups.

We also now make sure the key for the inserted AutoType is the same as the key used for lookup. Before, this was not the case, and it caused also non-deterministic parsing errors.

Fixes #110231

...in `ASTContext::getAutoTypeInternal` Given ```cpp template < typename > concept C1 = true; template < typename , auto > concept C2 = true; template < C1 auto V, C2< V > auto> struct S; ``` Both `C1 auto V` and `C2<V> auto` end on the set `AutoType`, the former being a template parameter for the latter. Since the hashing is not deterministic (i.e., pointers are hashed), every now and then, both will end on the same bucket. Given that `FoldingSet` recomputes the `FoldingSetID` for each node in the target bucket on lookup, this triggers an infinite recursion: 1. Look for `X` in `AutoTypes` 2. Let's assume it would be in bucket N, so it iterates over nodes in that bucket. Let's assume the first is `C2<V> auto`. 3. Computes the `FoldingSetID` for this one, which requires the profile of its template parameters, so they are visited. 4. In some frames below, we end on the same `FoldingSet`, and, by chance, `C1 auto V` would be in bucket N too. 5. But the first node in the bucket is `C2<V> auto` for which we need to profile `C1 auto V` 6. ... stack overflow! No step individually does anything wrong, but in general, `FoldingSet` seems not to be re-entrant, and this fact is hidden behind many nested calls. With this change, we store the `AutoType`s inside a `DenseMap` instead. The `FoldingSetID` is computed once only and then kept as the map's key, avoiding the need to do recursive lookups. We also now make sure the key for the inserted `AutoType` is the same as the key used for lookup. Before, this was not the case, and it caused also non-deterministic parsing errors. Fixes llvm#110231

graphite-app · 2024-12-02T12:57:44Z

Your org has enabled the Graphite merge queue for merging into main

Add the label “FP Bundles” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

llvmbot · 2024-12-02T12:58:18Z

@llvm/pr-subscribers-clang

Author: Alejandro Álvarez Ayllón (alejandro-alvarez-sonarsource)

Changes

...in ASTContext::getAutoTypeInternal

Given

template &lt; typename &gt;
concept C1 = true;

template &lt; typename , auto &gt;
concept C2 = true;

template &lt; C1 auto V, C2&lt; V &gt; auto&gt;
struct S;

Both C1 auto V and C2<V> auto end on the set AutoType, the former being a template parameter for the latter.

Since the hashing is not deterministic (i.e., pointers are hashed), every now and then, both will end on the same bucket. Given that FoldingSet recomputes the FoldingSetID for each node in the target bucket on lookup, this triggers an infinite recursion:

Look for X in AutoTypes
Let's assume it would be in bucket N, so it iterates over nodes in that bucket. Let's assume the first is C2<V> auto.
Computes the FoldingSetID for this one, which requires the profile of its template parameters, so they are visited.
In some frames below, we end on the same FoldingSet, and, by chance, C1 auto V would be in bucket N too.
But the first node in the bucket is C2<V> auto for which we need to profile C1 auto V
... stack overflow!

No step individually does anything wrong, but in general, FoldingSet seems not to be re-entrant, and this fact is hidden behind many nested calls.

With this change, we store the AutoTypes inside a DenseMap instead. The FoldingSetID is computed once only and then kept as the map's key, avoiding the need to do recursive lookups.

We also now make sure the key for the inserted AutoType is the same as the key used for lookup. Before, this was not the case, and it caused also non-deterministic parsing errors.

Fixes #110231

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

4 Files Affected:

(modified) clang/include/clang/AST/ASTContext.h (+5-1)
(modified) clang/include/clang/AST/Type.h (+1-1)
(modified) clang/lib/AST/ASTContext.cpp (+33-10)
(added) clang/test/Parser/gh110231.cpp (+12)

diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 89fcb6789d880a..1e89a6805ce9c6 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -245,7 +245,11 @@ class ASTContext : public RefCountedBase<ASTContext> {
   mutable llvm::FoldingSet<ObjCObjectPointerType> ObjCObjectPointerTypes;
   mutable llvm::FoldingSet<DependentUnaryTransformType>
     DependentUnaryTransformTypes;
-  mutable llvm::ContextualFoldingSet<AutoType, ASTContext&> AutoTypes;
+  // An AutoType can have a dependency on another AutoType via its template
+  // arguments. Since both dependent and dependency are on the same set,
+  // we can end up in an infinite recursion when looking for a node if we used
+  // a `FoldingSet`, since both could end up in the same bucket.
+  mutable llvm::DenseMap<llvm::FoldingSetNodeID, AutoType *> AutoTypes;
   mutable llvm::FoldingSet<DeducedTemplateSpecializationType>
     DeducedTemplateSpecializationTypes;
   mutable llvm::FoldingSet<AtomicType> AtomicTypes;
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 90a52b1dcbf624..7a10aedbcb6437 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -6551,7 +6551,7 @@ class DeducedType : public Type {
 
 /// Represents a C++11 auto or C++14 decltype(auto) type, possibly constrained
 /// by a type-constraint.
-class AutoType : public DeducedType, public llvm::FoldingSetNode {
+class AutoType : public DeducedType {
   friend class ASTContext; // ASTContext creates these
 
   ConceptDecl *TypeConstraintConcept;
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 80e8c5b9df58e7..f2cb9d3c57b5e9 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -112,6 +112,27 @@ enum FloatingRank {
   Ibm128Rank
 };
 
+template <> struct llvm::DenseMapInfo<llvm::FoldingSetNodeID> {
+  static FoldingSetNodeID getEmptyKey() { return FoldingSetNodeID{}; }
+
+  static FoldingSetNodeID getTombstoneKey() {
+    FoldingSetNodeID id;
+    for (size_t i = 0; i < sizeof(id) / sizeof(unsigned); ++i) {
+      id.AddInteger(std::numeric_limits<unsigned>::max());
+    }
+    return id;
+  }
+
+  static unsigned getHashValue(const FoldingSetNodeID &Val) {
+    return Val.ComputeHash();
+  }
+
+  static bool isEqual(const FoldingSetNodeID &LHS,
+                      const FoldingSetNodeID &RHS) {
+    return LHS == RHS;
+  }
+};
+
 /// \returns The locations that are relevant when searching for Doc comments
 /// related to \p D.
 static SmallVector<SourceLocation, 2>
@@ -899,7 +920,7 @@ ASTContext::ASTContext(LangOptions &LOpts, SourceManager &SM,
       FunctionProtoTypes(this_(), FunctionProtoTypesLog2InitSize),
       DependentTypeOfExprTypes(this_()), DependentDecltypeTypes(this_()),
       TemplateSpecializationTypes(this_()),
-      DependentTemplateSpecializationTypes(this_()), AutoTypes(this_()),
+      DependentTemplateSpecializationTypes(this_()),
       DependentBitIntTypes(this_()), SubstTemplateTemplateParmPacks(this_()),
       DeducedTemplates(this_()), ArrayParameterTypes(this_()),
       CanonTemplateTemplateParms(this_()), SourceMgr(SM), LangOpts(LOpts),
@@ -6294,12 +6315,13 @@ QualType ASTContext::getAutoTypeInternal(
     return getAutoDeductType();
 
   // Look in the folding set for an existing type.
-  void *InsertPos = nullptr;
   llvm::FoldingSetNodeID ID;
-  AutoType::Profile(ID, *this, DeducedType, Keyword, IsDependent,
+  bool IsDeducedDependent =
+      !DeducedType.isNull() && DeducedType->isDependentType();
+  AutoType::Profile(ID, *this, DeducedType, Keyword, IsDependent || IsDeducedDependent,
                     TypeConstraintConcept, TypeConstraintArgs);
-  if (AutoType *AT = AutoTypes.FindNodeOrInsertPos(ID, InsertPos))
-    return QualType(AT, 0);
+  if (auto const AT_iter = AutoTypes.find(ID); AT_iter != AutoTypes.end())
+    return QualType(AT_iter->getSecond(), 0);
 
   QualType Canon;
   if (!IsCanon) {
@@ -6314,10 +6336,6 @@ QualType ASTContext::getAutoTypeInternal(
         Canon =
             getAutoTypeInternal(QualType(), Keyword, IsDependent, IsPack,
                                 CanonicalConcept, CanonicalConceptArgs, true);
-        // Find the insert position again.
-        [[maybe_unused]] auto *Nothing =
-            AutoTypes.FindNodeOrInsertPos(ID, InsertPos);
-        assert(!Nothing && "canonical type broken");
       }
     }
   }
@@ -6331,8 +6349,13 @@ QualType ASTContext::getAutoTypeInternal(
                    : TypeDependence::None) |
           (IsPack ? TypeDependence::UnexpandedPack : TypeDependence::None),
       Canon, TypeConstraintConcept, TypeConstraintArgs);
+#ifndef NDEBUG
+  llvm::FoldingSetNodeID InsertedID;
+  AT->Profile(InsertedID, *this);
+  assert(InsertedID == ID && "ID does not match");
+#endif
   Types.push_back(AT);
-  AutoTypes.InsertNode(AT, InsertPos);
+  AutoTypes.try_emplace(ID, AT);
   return QualType(AT, 0);
 }
 
diff --git a/clang/test/Parser/gh110231.cpp b/clang/test/Parser/gh110231.cpp
new file mode 100644
index 00000000000000..b1405517505ff7
--- /dev/null
+++ b/clang/test/Parser/gh110231.cpp
@@ -0,0 +1,12 @@
+// RUN: seq 100 | xargs -Ifoo %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+// expected-no-diagnostics
+// This is a regression test for a non-deterministic stack-overflow.
+
+template < typename >
+concept C1 = true;
+
+template < typename , auto >
+concept C2 = true;
+
+template < C1 auto V, C2< V > auto>
+struct S;

github-actions · 2024-12-02T13:00:49Z

✅ With the latest revision this PR passed the C/C++ code formatter.

erichkeane

This seems like a reasonable solution for this, though I'd very much like to see the compile-time perf results for this before committing.

@nikic , is this something we can do quickly?

nikic · 2024-12-09T15:12:38Z

@erichkeane I'm not seeing any significant compile-time impact: https://llvm-compile-time-tracker.com/compare.php?from=e0ea9fd6dc36f585e364d4e569095ebe063e2573&to=7fb8c423fddd1a81287b54f48da4de1c81566fc1&stat=instructions:u

erichkeane · 2024-12-09T15:14:13Z

@erichkeane I'm not seeing any significant compile-time impact: https://llvm-compile-time-tracker.com/compare.php?from=e0ea9fd6dc36f585e364d4e569095ebe063e2573&to=7fb8c423fddd1a81287b54f48da4de1c81566fc1&stat=instructions:u

Perfect, thank you! LGTM!

alejandro-alvarez-sonarsource · 2024-12-10T08:00:50Z

Thanks! Can you do the merge? I do not have the rights.

Also, would it make sense to backport to 19?

lbonn · 2024-12-16T15:20:32Z

Given that the original report is a regression that prevents using the current version of mp-units, I would advocate for a backport.

As I understand, the procedure is to send the cherry-pick command that will be picked up by llvmbot so that the resulting MR can be considered for triage. However, I am hesitant to trigger it myself as I am neither an llvm contributor nor the author.

@alejandro-alvarez-sonarsource could you maybe trigger the bot?

nikic · 2024-12-16T15:35:02Z

This is an ABI-breaking fix, so it cannot be backported in this form.

hubert-reinterpretcast · 2024-12-20T00:06:21Z

This is an ABI-breaking fix, so it cannot be backported in this form.

@nikic, can you explain this statement? Is this something that can be documented in the release notes: https://github.com/llvm/llvm-project/commits/main/clang/docs/ReleaseNotes.rst?

nikic · 2024-12-20T09:59:18Z

@hubert-reinterpretcast This patch modifies structure layout in exported headers, changing the libclang-cpp ABI. LLVM patch releases cannot break API or ABI compatibility, see the last point in https://llvm.org/docs/HowToReleaseLLVM.html#release-patch-rules.

hubert-reinterpretcast · 2024-12-20T16:08:11Z

LLVM patch releases cannot break API or ABI compatibility

Thanks @nikic for the explanation. I thought you meant ABI compatibility in the "user program" sense.

[clang] Fix non-deterministic infinite recursion... (llvm#118288)

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 2, 2024

Fix formatting

6c7b60f

zyn0217 requested review from mizvekov, cor3ntin and erichkeane December 9, 2024 13:48

erichkeane approved these changes Dec 9, 2024

View reviewed changes

cor3ntin merged commit c2d7e96 into llvm:main Dec 10, 2024
8 checks passed

alejandro-alvarez-sonarsource deleted the aa/fix_gh110231 branch December 12, 2024 12:59

lbonn mentioned this pull request Dec 16, 2024

clang 19.1 does not support mp units (but 18 to 16 do) mpusz/mp-units#622

Closed

alejandro-alvarez-sonarsource mentioned this pull request Jan 8, 2025

[clang][Sema] Fix initialization of NonTypeTemplateParmDecl... #121768

Merged

lbonn mentioned this pull request Mar 19, 2025

Fail to compile v2.0.0 with MSVC mpusz/mp-units#518

Open

hanickadot added a commit to hanickadot/llvm-project that referenced this pull request May 4, 2025

Merge pull request #1 from mpusz/P3068-constexpr-exceptions

b99ff4d

[clang] Fix non-deterministic infinite recursion... (llvm#118288)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang] Fix non-deterministic infinite recursion... #118288

[clang] Fix non-deterministic infinite recursion... #118288

alejandro-alvarez-sonarsource commented Dec 2, 2024

graphite-app bot commented Dec 2, 2024

llvmbot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024 •

edited

Loading

erichkeane left a comment

nikic commented Dec 9, 2024

erichkeane commented Dec 9, 2024

alejandro-alvarez-sonarsource commented Dec 10, 2024

lbonn commented Dec 16, 2024

nikic commented Dec 16, 2024

hubert-reinterpretcast commented Dec 20, 2024

nikic commented Dec 20, 2024

hubert-reinterpretcast commented Dec 20, 2024

[clang] Fix non-deterministic infinite recursion... #118288

[clang] Fix non-deterministic infinite recursion... #118288

Conversation

alejandro-alvarez-sonarsource commented Dec 2, 2024

graphite-app bot commented Dec 2, 2024

Your org has enabled the Graphite merge queue for merging into main

llvmbot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024 • edited Loading

erichkeane left a comment

Choose a reason for hiding this comment

nikic commented Dec 9, 2024

erichkeane commented Dec 9, 2024

alejandro-alvarez-sonarsource commented Dec 10, 2024

lbonn commented Dec 16, 2024

nikic commented Dec 16, 2024

hubert-reinterpretcast commented Dec 20, 2024

nikic commented Dec 20, 2024

hubert-reinterpretcast commented Dec 20, 2024

github-actions bot commented Dec 2, 2024 •

edited

Loading