[clang] [Modules] Fix the inconsistency of which `Decl` should be serialized for an identifier. (PR #135887)
llvm-ci wrote: LLVM Buildbot has detected a new failure on builder `flang-x86_64-windows` running on `minipc-ryzen-win` while building `clang` at step 8 "test-build-unified-tree-check-flang-rt". Full details are available at: https://lab.llvm.org/buildbot/#/builders/166/builds/1158 Here is the relevant piece of the build log for the reference ``` Step 8 (test-build-unified-tree-check-flang-rt) failure: test (failure) TEST 'flang-rt-OldUnit :: Runtime/RuntimeTests.exe' FAILED [==] Running 225 tests from 54 test suites. [--] Global test environment set-up. [--] 4 tests from AllocatableTest [ RUN ] AllocatableTest.MoveAlloc [ OK ] AllocatableTest.MoveAlloc (0 ms) [ RUN ] AllocatableTest.AllocateFromScalarSource [ OK ] AllocatableTest.AllocateFromScalarSource (0 ms) [ RUN ] AllocatableTest.AllocateSourceZeroSize [ OK ] AllocatableTest.AllocateSourceZeroSize (0 ms) [ RUN ] AllocatableTest.DoubleAllocation [ OK ] AllocatableTest.DoubleAllocation (0 ms) [--] 4 tests from AllocatableTest (0 ms total) [--] 3 tests from ArrayConstructor [ RUN ] ArrayConstructor.Basic [ OK ] ArrayConstructor.Basic (0 ms) [ RUN ] ArrayConstructor.Character [ OK ] ArrayConstructor.Character (0 ms) [ RUN ] ArrayConstructor.CharacterRuntimeCheck [ OK ] ArrayConstructor.CharacterRuntimeCheck (72 ms) [--] 3 tests from ArrayConstructor (72 ms total) [--] 1 test from BufferTests [ RUN ] BufferTests.TestFrameBufferReadAndWrite [ OK ] BufferTests.TestFrameBufferReadAndWrite (0 ms) ... ``` https://github.com/llvm/llvm-project/pull/135887 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Fix the inconsistency of which `Decl` should be serialized for an identifier. (PR #135887)
https://github.com/vsapsai closed https://github.com/llvm/llvm-project/pull/135887 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Fix the inconsistency of which `Decl` should be serialized for an identifier. (PR #135887)
https://github.com/vsapsai updated
https://github.com/llvm/llvm-project/pull/135887
>From d944252ce2f08d1522f7297e4d7f9a7b730db180 Mon Sep 17 00:00:00 2001
From: Volodymyr Sapsai
Date: Tue, 15 Apr 2025 16:34:47 -0700
Subject: [PATCH 1/2] [Modules] Fix the inconsistency of which `Decl` should be
serialized for an identifier.
Fixes the assertion failure
> Assertion failed: (DeclIDs.contains(D) && "Declaration not emitted!"),
> function getDeclID, file ASTWriter.cpp, line 6873.
We prepare to serialize a `Decl` by adding it to `DeclIDs` in
`ASTWriter::GetDeclRef`. But the checks before this call aren't the same as
when we are actually serializing a `Decl` in `ASTIdentifierTableTrait::EmitData`
and `ASTWriter::WriteIdentifierTable`. That's how we can end up serializing
a `Decl` not present in `DeclIDs` and hitting the assertion. With the assertions
disabled clang crashes when trying to use a deserialized null `Decl`.
Fix by making the code checks before `ASTWriter::GetDeclRef` call similar
to those we have before the serialization.
rdar://139319683
---
clang/include/clang/Serialization/ASTWriter.h | 1 +
clang/lib/Serialization/ASTWriter.cpp | 11 +-
clang/test/Modules/non-modular-decl-use.c | 100 ++
3 files changed, 109 insertions(+), 3 deletions(-)
create mode 100644 clang/test/Modules/non-modular-decl-use.c
diff --git a/clang/include/clang/Serialization/ASTWriter.h
b/clang/include/clang/Serialization/ASTWriter.h
index bdf3aca0637c8..5075232596cea 100644
--- a/clang/include/clang/Serialization/ASTWriter.h
+++ b/clang/include/clang/Serialization/ASTWriter.h
@@ -224,6 +224,7 @@ class ASTWriter : public ASTDeserializationListener,
/// discovery) and start at 2. 1 is reserved for the translation
/// unit, while 0 is reserved for NULL.
llvm::DenseMap DeclIDs;
+ // TMP: ^ DeclIDs type for reference
/// Set of predefined decls. This is a helper data to determine if a decl
/// is predefined. It should be more clear and safer to query the set
diff --git a/clang/lib/Serialization/ASTWriter.cpp
b/clang/lib/Serialization/ASTWriter.cpp
index 95b5718f1d140..c9db69b3b784b 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -3925,6 +3925,7 @@ class ASTIdentifierTableTrait {
// Only emit declarations that aren't from a chained PCH, though.
SmallVector Decls(IdResolver->decls(II));
for (NamedDecl *D : llvm::reverse(Decls))
+// TMP: corresponding `getDeclForLocalLookup` call
LE.write((DeclID)Writer.getDeclID(
getDeclForLocalLookup(PP.getLangOpts(), D)));
}
@@ -3969,6 +3970,7 @@ void ASTWriter::WriteIdentifierTable(Preprocessor &PP,
if (isLocalIdentifierID(ID) || II->hasChangedSinceDeserialization() ||
(Trait.needDecls() &&
II->hasFETokenInfoChangedSinceDeserialization()))
+// TMP: ^ corresponding `hasFETokenInfoChangedSinceDeserialization`
call
Generator.insert(II, ID, Trait);
}
@@ -5664,14 +5666,16 @@ void ASTWriter::PrepareWritingSpecialDecls(Sema
&SemaRef) {
llvm::SmallVector IIs;
for (const auto &ID : SemaRef.PP.getIdentifierTable()) {
const IdentifierInfo *II = ID.second;
- if (!Chain || !II->isFromAST() || II->hasChangedSinceDeserialization())
+ if (!Chain || !II->isFromAST() || II->hasChangedSinceDeserialization() ||
+ II->hasFETokenInfoChangedSinceDeserialization())
IIs.push_back(II);
}
// Sort the identifiers to visit based on their name.
llvm::sort(IIs, llvm::deref>());
+const LangOptions &LangOpts = getLangOpts();
for (const IdentifierInfo *II : IIs)
- for (const Decl *D : SemaRef.IdResolver.decls(II))
-GetDeclRef(D);
+ for (NamedDecl *D : SemaRef.IdResolver.decls(II))
+GetDeclRef(getDeclForLocalLookup(LangOpts, D));
}
// Write all of the DeclsToCheckForDeferredDiags.
@@ -6845,6 +6849,7 @@ LocalDeclID ASTWriter::GetDeclRef(const Decl *D) {
}
assert(!(reinterpret_cast(D) & 0x01) && "Invalid decl pointer");
+ // TMP: adding D to DeclIDs
LocalDeclID &ID = DeclIDs[D];
if (ID.isInvalid()) {
if (DoneWritingDeclsAndTypes) {
diff --git a/clang/test/Modules/non-modular-decl-use.c
b/clang/test/Modules/non-modular-decl-use.c
new file mode 100644
index 0..1bd87bf284620
--- /dev/null
+++ b/clang/test/Modules/non-modular-decl-use.c
@@ -0,0 +1,100 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -I %t/include %t/test.c \
+// RUN:-fmodules -fimplicit-module-maps
-fmodules-cache-path=%t/modules.cache
+
+// Test when a decl is present in multiple modules through an inclusion of
+// a non-modular header. Make sure such decl is serialized correctly and can be
+// used after deserialization.
+
+//--- include/non-modular.h
+#ifndef NON_MODULAR_H
+#define NON_MODULAR_H
+
+union TestUnion {
+ int x;
+ float y;
+};
+
+struct ReferenceUnion1 {
[clang] [Modules] Fix the inconsistency of which `Decl` should be serialized for an identifier. (PR #135887)
https://github.com/jansvoboda11 approved this pull request. https://github.com/llvm/llvm-project/pull/135887 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Fix the inconsistency of which `Decl` should be serialized for an identifier. (PR #135887)
https://github.com/ChuanqiXu9 approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/135887 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Fix the inconsistency of which `Decl` should be serialized for an identifier. (PR #135887)
vsapsai wrote: cc @zygoloid as he has touched this code 10 years ago. https://github.com/llvm/llvm-project/pull/135887 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Fix the inconsistency of which `Decl` should be serialized for an identifier. (PR #135887)
@@ -224,6 +224,7 @@ class ASTWriter : public ASTDeserializationListener, /// discovery) and start at 2. 1 is reserved for the translation /// unit, while 0 is reserved for NULL. llvm::DenseMap DeclIDs; + // TMP: ^ DeclIDs type for reference vsapsai wrote: I will remove all TMP comments before committing. Using this to simplify the review by pulling in the pointers to the relevant code. https://github.com/llvm/llvm-project/pull/135887 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Fix the inconsistency of which `Decl` should be serialized for an identifier. (PR #135887)
llvmbot wrote:
@llvm/pr-subscribers-clang-modules
@llvm/pr-subscribers-clang
Author: Volodymyr Sapsai (vsapsai)
Changes
Fixes the assertion failure
> Assertion failed: (DeclIDs.contains(D) && "Declaration not
emitted!"), function getDeclID, file ASTWriter.cpp, line 6873.
We prepare to serialize a `Decl` by adding it to `DeclIDs` in
`ASTWriter::GetDeclRef`. But the checks before this call aren't the same as
when we are actually serializing a `Decl` in
`ASTIdentifierTableTrait::EmitData` and `ASTWriter::WriteIdentifierTable`.
That's how we can end up serializing a `Decl` not present in `DeclIDs` and
hitting the assertion. With the assertions disabled clang crashes when trying
to use a deserialized null `Decl`.
Fix by making the code checks before `ASTWriter::GetDeclRef` call similar to
those we have before the serialization.
rdar://139319683
---
Full diff: https://github.com/llvm/llvm-project/pull/135887.diff
3 Files Affected:
- (modified) clang/include/clang/Serialization/ASTWriter.h (+1)
- (modified) clang/lib/Serialization/ASTWriter.cpp (+8-3)
- (added) clang/test/Modules/non-modular-decl-use.c (+100)
``diff
diff --git a/clang/include/clang/Serialization/ASTWriter.h
b/clang/include/clang/Serialization/ASTWriter.h
index bdf3aca0637c8..5075232596cea 100644
--- a/clang/include/clang/Serialization/ASTWriter.h
+++ b/clang/include/clang/Serialization/ASTWriter.h
@@ -224,6 +224,7 @@ class ASTWriter : public ASTDeserializationListener,
/// discovery) and start at 2. 1 is reserved for the translation
/// unit, while 0 is reserved for NULL.
llvm::DenseMap DeclIDs;
+ // TMP: ^ DeclIDs type for reference
/// Set of predefined decls. This is a helper data to determine if a decl
/// is predefined. It should be more clear and safer to query the set
diff --git a/clang/lib/Serialization/ASTWriter.cpp
b/clang/lib/Serialization/ASTWriter.cpp
index 95b5718f1d140..c9db69b3b784b 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -3925,6 +3925,7 @@ class ASTIdentifierTableTrait {
// Only emit declarations that aren't from a chained PCH, though.
SmallVector Decls(IdResolver->decls(II));
for (NamedDecl *D : llvm::reverse(Decls))
+// TMP: corresponding `getDeclForLocalLookup` call
LE.write((DeclID)Writer.getDeclID(
getDeclForLocalLookup(PP.getLangOpts(), D)));
}
@@ -3969,6 +3970,7 @@ void ASTWriter::WriteIdentifierTable(Preprocessor &PP,
if (isLocalIdentifierID(ID) || II->hasChangedSinceDeserialization() ||
(Trait.needDecls() &&
II->hasFETokenInfoChangedSinceDeserialization()))
+// TMP: ^ corresponding `hasFETokenInfoChangedSinceDeserialization`
call
Generator.insert(II, ID, Trait);
}
@@ -5664,14 +5666,16 @@ void ASTWriter::PrepareWritingSpecialDecls(Sema
&SemaRef) {
llvm::SmallVector IIs;
for (const auto &ID : SemaRef.PP.getIdentifierTable()) {
const IdentifierInfo *II = ID.second;
- if (!Chain || !II->isFromAST() || II->hasChangedSinceDeserialization())
+ if (!Chain || !II->isFromAST() || II->hasChangedSinceDeserialization() ||
+ II->hasFETokenInfoChangedSinceDeserialization())
IIs.push_back(II);
}
// Sort the identifiers to visit based on their name.
llvm::sort(IIs, llvm::deref>());
+const LangOptions &LangOpts = getLangOpts();
for (const IdentifierInfo *II : IIs)
- for (const Decl *D : SemaRef.IdResolver.decls(II))
-GetDeclRef(D);
+ for (NamedDecl *D : SemaRef.IdResolver.decls(II))
+GetDeclRef(getDeclForLocalLookup(LangOpts, D));
}
// Write all of the DeclsToCheckForDeferredDiags.
@@ -6845,6 +6849,7 @@ LocalDeclID ASTWriter::GetDeclRef(const Decl *D) {
}
assert(!(reinterpret_cast(D) & 0x01) && "Invalid decl pointer");
+ // TMP: adding D to DeclIDs
LocalDeclID &ID = DeclIDs[D];
if (ID.isInvalid()) {
if (DoneWritingDeclsAndTypes) {
diff --git a/clang/test/Modules/non-modular-decl-use.c
b/clang/test/Modules/non-modular-decl-use.c
new file mode 100644
index 0..1bd87bf284620
--- /dev/null
+++ b/clang/test/Modules/non-modular-decl-use.c
@@ -0,0 +1,100 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -I %t/include %t/test.c \
+// RUN:-fmodules -fimplicit-module-maps
-fmodules-cache-path=%t/modules.cache
+
+// Test when a decl is present in multiple modules through an inclusion of
+// a non-modular header. Make sure such decl is serialized correctly and can be
+// used after deserialization.
+
+//--- include/non-modular.h
+#ifndef NON_MODULAR_H
+#define NON_MODULAR_H
+
+union TestUnion {
+ int x;
+ float y;
+};
+
+struct ReferenceUnion1 {
+ union TestUnion name;
+ unsigned versionMajor;
+};
+struct ReferenceUnion2 {
+ union TestUnion name;
+ unsigned versionMinor;
+};
+
+// Test another kind of RecordDecl.
+struct TestStruct {
+ int p;
+
[clang] [Modules] Fix the inconsistency of which `Decl` should be serialized for an identifier. (PR #135887)
https://github.com/vsapsai created
https://github.com/llvm/llvm-project/pull/135887
Fixes the assertion failure
> Assertion failed: (DeclIDs.contains(D) && "Declaration not emitted!"),
> function getDeclID, file ASTWriter.cpp, line 6873.
We prepare to serialize a `Decl` by adding it to `DeclIDs` in
`ASTWriter::GetDeclRef`. But the checks before this call aren't the same as
when we are actually serializing a `Decl` in
`ASTIdentifierTableTrait::EmitData` and `ASTWriter::WriteIdentifierTable`.
That's how we can end up serializing a `Decl` not present in `DeclIDs` and
hitting the assertion. With the assertions disabled clang crashes when trying
to use a deserialized null `Decl`.
Fix by making the code checks before `ASTWriter::GetDeclRef` call similar to
those we have before the serialization.
rdar://139319683
>From d944252ce2f08d1522f7297e4d7f9a7b730db180 Mon Sep 17 00:00:00 2001
From: Volodymyr Sapsai
Date: Tue, 15 Apr 2025 16:34:47 -0700
Subject: [PATCH] [Modules] Fix the inconsistency of which `Decl` should be
serialized for an identifier.
Fixes the assertion failure
> Assertion failed: (DeclIDs.contains(D) && "Declaration not emitted!"),
> function getDeclID, file ASTWriter.cpp, line 6873.
We prepare to serialize a `Decl` by adding it to `DeclIDs` in
`ASTWriter::GetDeclRef`. But the checks before this call aren't the same as
when we are actually serializing a `Decl` in `ASTIdentifierTableTrait::EmitData`
and `ASTWriter::WriteIdentifierTable`. That's how we can end up serializing
a `Decl` not present in `DeclIDs` and hitting the assertion. With the assertions
disabled clang crashes when trying to use a deserialized null `Decl`.
Fix by making the code checks before `ASTWriter::GetDeclRef` call similar
to those we have before the serialization.
rdar://139319683
---
clang/include/clang/Serialization/ASTWriter.h | 1 +
clang/lib/Serialization/ASTWriter.cpp | 11 +-
clang/test/Modules/non-modular-decl-use.c | 100 ++
3 files changed, 109 insertions(+), 3 deletions(-)
create mode 100644 clang/test/Modules/non-modular-decl-use.c
diff --git a/clang/include/clang/Serialization/ASTWriter.h
b/clang/include/clang/Serialization/ASTWriter.h
index bdf3aca0637c8..5075232596cea 100644
--- a/clang/include/clang/Serialization/ASTWriter.h
+++ b/clang/include/clang/Serialization/ASTWriter.h
@@ -224,6 +224,7 @@ class ASTWriter : public ASTDeserializationListener,
/// discovery) and start at 2. 1 is reserved for the translation
/// unit, while 0 is reserved for NULL.
llvm::DenseMap DeclIDs;
+ // TMP: ^ DeclIDs type for reference
/// Set of predefined decls. This is a helper data to determine if a decl
/// is predefined. It should be more clear and safer to query the set
diff --git a/clang/lib/Serialization/ASTWriter.cpp
b/clang/lib/Serialization/ASTWriter.cpp
index 95b5718f1d140..c9db69b3b784b 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -3925,6 +3925,7 @@ class ASTIdentifierTableTrait {
// Only emit declarations that aren't from a chained PCH, though.
SmallVector Decls(IdResolver->decls(II));
for (NamedDecl *D : llvm::reverse(Decls))
+// TMP: corresponding `getDeclForLocalLookup` call
LE.write((DeclID)Writer.getDeclID(
getDeclForLocalLookup(PP.getLangOpts(), D)));
}
@@ -3969,6 +3970,7 @@ void ASTWriter::WriteIdentifierTable(Preprocessor &PP,
if (isLocalIdentifierID(ID) || II->hasChangedSinceDeserialization() ||
(Trait.needDecls() &&
II->hasFETokenInfoChangedSinceDeserialization()))
+// TMP: ^ corresponding `hasFETokenInfoChangedSinceDeserialization`
call
Generator.insert(II, ID, Trait);
}
@@ -5664,14 +5666,16 @@ void ASTWriter::PrepareWritingSpecialDecls(Sema
&SemaRef) {
llvm::SmallVector IIs;
for (const auto &ID : SemaRef.PP.getIdentifierTable()) {
const IdentifierInfo *II = ID.second;
- if (!Chain || !II->isFromAST() || II->hasChangedSinceDeserialization())
+ if (!Chain || !II->isFromAST() || II->hasChangedSinceDeserialization() ||
+ II->hasFETokenInfoChangedSinceDeserialization())
IIs.push_back(II);
}
// Sort the identifiers to visit based on their name.
llvm::sort(IIs, llvm::deref>());
+const LangOptions &LangOpts = getLangOpts();
for (const IdentifierInfo *II : IIs)
- for (const Decl *D : SemaRef.IdResolver.decls(II))
-GetDeclRef(D);
+ for (NamedDecl *D : SemaRef.IdResolver.decls(II))
+GetDeclRef(getDeclForLocalLookup(LangOpts, D));
}
// Write all of the DeclsToCheckForDeferredDiags.
@@ -6845,6 +6849,7 @@ LocalDeclID ASTWriter::GetDeclRef(const Decl *D) {
}
assert(!(reinterpret_cast(D) & 0x01) && "Invalid decl pointer");
+ // TMP: adding D to DeclIDs
LocalDeclID &ID = DeclIDs[D];
if (ID.isInvalid()) {
if (DoneWritingDeclsAndTypes) {
diff --git a/clang/test/Modules/n
