martong created this revision.
martong added reviewers: vabridgers, riccibruno, aaron.ballman.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs.
Herald added a project: clang.
https://github.com/llvm/llvm-project/commit/05843dc6ab97a00cbde7aa4f08bf3692eb83109d
introduces a regression: deserialization a function crashes if it has a switch
and a lambda with a switch (see the test case).
In this case the AST contains the exact same SwitchStmt both in the lambda's
CXXMethodDecl and as a child of the LambdaExpr:
-LambdaExpr 0x55fc0fb0b4a0 <col:12, line:22:3>
|-CXXRecordDecl 0x55fc0fb0afc8 <line:19:12> col:12 implicit class definition
| |-CXXMethodDecl 0x55fc0fb0b108 <col:14, line:22:3> line:19:12 constexpr
operator() 'auto () const -> void' inline
| | `-CompoundStmt 0x55fc0fb0b2b0 <col:16, line:22:3>
| | `-SwitchStmt 0x55fc0fb0b208 <line:20:5, line:21:12>
`-CompoundStmt 0x55fc0fb0b2b0 <col:16, line:22:3>
`-SwitchStmt 0x55fc0fb0b208 <line:20:5, line:21:12>
During the serialization, each SwitchCase gets an id that is unique in a
function.
Durint the deserialization, we fail to properly clear the existing ids, thus we
receive the
Assertion `(*CurrSwitchCaseStmts)[ID] == nullptr && "Already have a
SwitchCase with this ID"' failed
This patch introduces a solution which removes the enforcment of unique ids per
function.
We track the ids and make them unique per PCH.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D82940
Files:
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriterDecl.cpp
clang/lib/Serialization/ASTWriterStmt.cpp
clang/test/AST/ast-dump-lambda.cpp
Index: clang/test/AST/ast-dump-lambda.cpp
===================================================================
--- clang/test/AST/ast-dump-lambda.cpp
+++ clang/test/AST/ast-dump-lambda.cpp
@@ -34,6 +34,21 @@
[]() noexcept {};
[]() -> int { return 0; };
}
+
+class {
+public:
+ enum {} a;
+} b;
+void should_not_crash_with_switch_in_lambda() {
+ switch (b.a)
+ default:;
+ enum { e } d;
+ auto f = [d] {
+ switch (d)
+ case e:;
+ };
+}
+
// CHECK:Dumping test:
// CHECK-NEXT:FunctionTemplateDecl {{.*}} <{{.*}}ast-dump-lambda.cpp:15:1,
line:36:1> line:15:32{{( imported)?}} test
// CHECK-NEXT:|-TemplateTypeParmDecl {{.*}} <col:11, col:23> col:23{{(
imported)?}} referenced typename depth 0 index 0 ... Ts
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===================================================================
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -2611,11 +2611,12 @@
//===----------------------------------------------------------------------===//
unsigned ASTWriter::RecordSwitchCaseID(SwitchCase *S) {
- assert(SwitchCaseIDs.find(S) == SwitchCaseIDs.end() &&
- "SwitchCase recorded twice");
unsigned NextID = SwitchCaseIDs.size();
- SwitchCaseIDs[S] = NextID;
- return NextID;
+ assert(NextID < UINT_MAX && "Too many SwitchCase to serialize");
+ // If we already recorded this SwitchCase then just return with its ID, else
+ // insert it with the next ID.
+ auto Res = SwitchCaseIDs.insert({S, NextID});
+ return Res.first->second;
}
unsigned ASTWriter::getSwitchCaseID(SwitchCase *S) {
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===================================================================
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -2453,9 +2453,6 @@
}
void ASTRecordWriter::AddFunctionDefinition(const FunctionDecl *FD) {
- // Switch case IDs are per function body.
- Writer->ClearSwitchCaseIDs();
-
assert(FD->doesThisDeclarationHaveABody());
bool ModulesCodegen = false;
if (Writer->WritingModule && !FD->isDependentContext()) {
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -7461,8 +7461,6 @@
/// source each time it is called, and is meant to be used via a
/// LazyOffsetPtr (which is used by Decls for the body of functions, etc).
Stmt *ASTReader::GetExternalDeclStmt(uint64_t Offset) {
- // Switch case IDs are per Decl.
- ClearSwitchCaseIDs();
// Offset here is a global offset across the entire chain.
RecordLocation Loc = getLocalBitOffset(Offset);
@@ -9067,8 +9065,7 @@
/// Record that the given ID maps to the given switch-case
/// statement.
void ASTReader::RecordSwitchCaseID(SwitchCase *SC, unsigned ID) {
- assert((*CurrSwitchCaseStmts)[ID] == nullptr &&
- "Already have a SwitchCase with this ID");
+ // We may already have a SwitchCase with this ID, drop the existing.
(*CurrSwitchCaseStmts)[ID] = SC;
}
Index: clang/test/AST/ast-dump-lambda.cpp
===================================================================
--- clang/test/AST/ast-dump-lambda.cpp
+++ clang/test/AST/ast-dump-lambda.cpp
@@ -34,6 +34,21 @@
[]() noexcept {};
[]() -> int { return 0; };
}
+
+class {
+public:
+ enum {} a;
+} b;
+void should_not_crash_with_switch_in_lambda() {
+ switch (b.a)
+ default:;
+ enum { e } d;
+ auto f = [d] {
+ switch (d)
+ case e:;
+ };
+}
+
// CHECK:Dumping test:
// CHECK-NEXT:FunctionTemplateDecl {{.*}} <{{.*}}ast-dump-lambda.cpp:15:1, line:36:1> line:15:32{{( imported)?}} test
// CHECK-NEXT:|-TemplateTypeParmDecl {{.*}} <col:11, col:23> col:23{{( imported)?}} referenced typename depth 0 index 0 ... Ts
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===================================================================
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -2611,11 +2611,12 @@
//===----------------------------------------------------------------------===//
unsigned ASTWriter::RecordSwitchCaseID(SwitchCase *S) {
- assert(SwitchCaseIDs.find(S) == SwitchCaseIDs.end() &&
- "SwitchCase recorded twice");
unsigned NextID = SwitchCaseIDs.size();
- SwitchCaseIDs[S] = NextID;
- return NextID;
+ assert(NextID < UINT_MAX && "Too many SwitchCase to serialize");
+ // If we already recorded this SwitchCase then just return with its ID, else
+ // insert it with the next ID.
+ auto Res = SwitchCaseIDs.insert({S, NextID});
+ return Res.first->second;
}
unsigned ASTWriter::getSwitchCaseID(SwitchCase *S) {
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===================================================================
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -2453,9 +2453,6 @@
}
void ASTRecordWriter::AddFunctionDefinition(const FunctionDecl *FD) {
- // Switch case IDs are per function body.
- Writer->ClearSwitchCaseIDs();
-
assert(FD->doesThisDeclarationHaveABody());
bool ModulesCodegen = false;
if (Writer->WritingModule && !FD->isDependentContext()) {
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -7461,8 +7461,6 @@
/// source each time it is called, and is meant to be used via a
/// LazyOffsetPtr (which is used by Decls for the body of functions, etc).
Stmt *ASTReader::GetExternalDeclStmt(uint64_t Offset) {
- // Switch case IDs are per Decl.
- ClearSwitchCaseIDs();
// Offset here is a global offset across the entire chain.
RecordLocation Loc = getLocalBitOffset(Offset);
@@ -9067,8 +9065,7 @@
/// Record that the given ID maps to the given switch-case
/// statement.
void ASTReader::RecordSwitchCaseID(SwitchCase *SC, unsigned ID) {
- assert((*CurrSwitchCaseStmts)[ID] == nullptr &&
- "Already have a SwitchCase with this ID");
+ // We may already have a SwitchCase with this ID, drop the existing.
(*CurrSwitchCaseStmts)[ID] = SC;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
