[PATCH] D82940: [ASTReader][ASTWriter][PCH][Modules] Fix (de)serialization of SwitchCases

[Gabor Marton via Phabricator via cfe-commits]

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;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D82940.274727.patch
Type: text/x-patch
Size: 3113 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200701/2d585803/attachment-0001.bin>