From b31a3f86d3427405f1182d59c381e90c93e521b8 Mon Sep 17 00:00:00 2001 From: Maksim Ivanov Date: Wed, 30 Apr 2025 00:31:30 +0000 Subject: [PATCH 1/4] [clang] Fix nondeterminism in MemberPointerType This commit fixes the nondeterminism issue in C++ header module enabled builds which were observed after https://github.com/llvm/llvm-project/pull/132401. The issue was related to the fact that the hash set operation in MemberPointerType::Profile() was triggering getMostRecentDecl(). The root cause seems to be that the latter was leading to the reentrant modification of the hash set, with some probability (likely depending on the actual values of hashes). We haven't been able to come up with a deterministic regression test for this fix. --- clang/include/clang/AST/Type.h | 3 ++- clang/lib/AST/Type.cpp | 10 +++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h index 3e1fb05ad537c..c93cbae767db8 100644 --- a/clang/include/clang/AST/Type.h +++ b/clang/include/clang/AST/Type.h @@ -3602,6 +3602,7 @@ class MemberPointerType : public Type, public llvm::FoldingSetNode { } NestedNameSpecifier *getQualifier() const { return Qualifier; } + CXXRecordDecl *getCXXRecordDecl() const; CXXRecordDecl *getMostRecentCXXRecordDecl() const; bool isSugared() const; @@ -3610,7 +3611,7 @@ class MemberPointerType : public Type, public llvm::FoldingSetNode { } void Profile(llvm::FoldingSetNodeID &ID) { - Profile(ID, getPointeeType(), getQualifier(), getMostRecentCXXRecordDecl()); + Profile(ID, getPointeeType(), getQualifier(), getCXXRecordDecl()); } static void Profile(llvm::FoldingSetNodeID &ID, QualType Pointee, diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp index 111a642173418..a149eac7e4555 100644 --- a/clang/lib/AST/Type.cpp +++ b/clang/lib/AST/Type.cpp @@ -5275,10 +5275,14 @@ void MemberPointerType::Profile(llvm::FoldingSetNodeID &ID, QualType Pointee, ID.AddPointer(Cls->getCanonicalDecl()); } +CXXRecordDecl *MemberPointerType::getCXXRecordDecl() const { + return dyn_cast(getCanonicalTypeInternal()) + ->getQualifier() + ->getAsRecordDecl(); +} + CXXRecordDecl *MemberPointerType::getMostRecentCXXRecordDecl() const { - auto *RD = dyn_cast(getCanonicalTypeInternal()) - ->getQualifier() - ->getAsRecordDecl(); + auto *RD = getCXXRecordDecl(); if (!RD) return nullptr; return RD->getMostRecentNonInjectedDecl(); From a8c958e79b7595b1d741b61357e3954af84affdf Mon Sep 17 00:00:00 2001 From: Maksim Ivanov Date: Wed, 30 Apr 2025 14:29:28 +0000 Subject: [PATCH 2/4] add comment --- clang/include/clang/AST/Type.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h index c93cbae767db8..6d08a2f49f9f9 100644 --- a/clang/include/clang/AST/Type.h +++ b/clang/include/clang/AST/Type.h @@ -3603,6 +3603,9 @@ class MemberPointerType : public Type, public llvm::FoldingSetNode { NestedNameSpecifier *getQualifier() const { return Qualifier; } CXXRecordDecl *getCXXRecordDecl() const; + /// Note: this can trigger extra deserialization when external AST sources are + /// used. Prefer `getCXXRecordDecl()` unless you really need the most recent + /// decl. CXXRecordDecl *getMostRecentCXXRecordDecl() const; bool isSugared() const; From 21c7ab129af7f73e6ecf82914b6f229ad06307c2 Mon Sep 17 00:00:00 2001 From: Maksim Ivanov Date: Mon, 5 May 2025 12:07:08 +0000 Subject: [PATCH 3/4] make new accessor private --- clang/include/clang/AST/Type.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h index 6d08a2f49f9f9..d24fb72190cdd 100644 --- a/clang/include/clang/AST/Type.h +++ b/clang/include/clang/AST/Type.h @@ -3602,7 +3602,6 @@ class MemberPointerType : public Type, public llvm::FoldingSetNode { } NestedNameSpecifier *getQualifier() const { return Qualifier; } - CXXRecordDecl *getCXXRecordDecl() const; /// Note: this can trigger extra deserialization when external AST sources are /// used. Prefer `getCXXRecordDecl()` unless you really need the most recent /// decl. @@ -3624,6 +3623,9 @@ class MemberPointerType : public Type, public llvm::FoldingSetNode { static bool classof(const Type *T) { return T->getTypeClass() == MemberPointer; } + +private: + CXXRecordDecl *getCXXRecordDecl() const; }; /// Capture whether this is a normal array (e.g. int X[4]) From a4bbcc31f1056892e06d03158abc4cbb4419a4f4 Mon Sep 17 00:00:00 2001 From: Maksim Ivanov Date: Mon, 5 May 2025 13:40:15 +0000 Subject: [PATCH 4/4] add FIXME --- clang/include/clang/AST/Type.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h index d24fb72190cdd..02a6fb5333538 100644 --- a/clang/include/clang/AST/Type.h +++ b/clang/include/clang/AST/Type.h @@ -3613,6 +3613,9 @@ class MemberPointerType : public Type, public llvm::FoldingSetNode { } void Profile(llvm::FoldingSetNodeID &ID) { + // FIXME: `getMostRecentCXXRecordDecl()` should be possible to use here, + // however when external AST sources are used it causes nondeterminism + // issues (see https://github.com/llvm/llvm-project/pull/137910). Profile(ID, getPointeeType(), getQualifier(), getCXXRecordDecl()); }