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

[martong]

Summary

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.

[aaron.ballman]

LGTM, thank you for the fix!

[riccibruno]

Let me take a look.

Reduced test case:

class C {} b;
void should_not_crash_with_switch_in_lambda() {
  switch (1)
  default:;
  auto f = [] {
    switch (1)
    default:;
  };
}

[riccibruno]

In D82940#2124955, @aaron.ballman wrote:

LGTM, thank you for the fix!

Wait, I am not sure that it is the right fix. The body of the lambda should not be different from the body of the CXXMethodDecl.

[aaron.ballman]

In D82940#2125005, @riccibruno wrote:

In D82940#2124955, @aaron.ballman wrote:

LGTM, thank you for the fix!

Wait, I am not sure that it is the right fix. The body of the lambda should not be different from the body of the CXXMethodDecl.

I'm not certain I understand, but I'm removing my LG while we figure it out. Can you expound a bit further?

[vabridgers]

This patch addresses a crash we started seeing in our CTU analysis runs as a result of 05843dc6ab97a00cbde7aa4f08bf3692eb83109d. Gabor, maybe I can generate a regression test for the problems that 05843dc6ab97a00cbde7aa4f08bf3692eb83109d caused as a follow up for whatever is committed here?

[riccibruno]

I'm not certain I understand, but I'm removing my LG while we figure it out. Can you expound a bit further?

Take a look at the AST dump of a small modification of the reduced test case which doesn't trigger the assertion:

void should_not_crash_with_switch_in_lambda() {
  switch (1)
  default:;
  auto f = [] {
    switch (1)
    default:;
  };
}

Before the serialization of LambdaExpr for the LambdaExpr *E, we have E->getBody() == E->getCallOperator()->getBody().
After serialization this is not true anymore because we are writing and reading the body directly when visiting the LambdaExpr
(the captures have the same problem).

[aaron.ballman]

In D82940#2125183, @riccibruno wrote:

I'm not certain I understand, but I'm removing my LG while we figure it out. Can you expound a bit further?

Take a look at the AST dump of a small modification of the reduced test case which doesn't trigger the assertion:

void should_not_crash_with_switch_in_lambda() {
  switch (1)
  default:;
  auto f = [] {
    switch (1)
    default:;
  };
}

Before the serialization of LambdaExpr for the LambdaExpr *E, we have E->getBody() == E->getCallOperator()->getBody().
After serialization this is not true anymore because we are writing and reading the body directly when visiting the LambdaExpr
(the captures have the same problem).

Ah, thank you for the explanation! That does seem like an issue, good catch.

[martong]

In case of the reproducer the problem is that we clear the switch ids in GetExternalDeclStmt. But we read the SwitchCase with the same ID twice from the same call of GetExternalDeclStmt (bold-italic in the below call stacks):

• 1)

clang::ASTReader::RecordSwitchCaseID (this=0x4d5710, SC=0x5022f0, ID=0) at ../../git/llvm-project/clang/lib/Serialization/ASTReader.cpp:9070
0x00007ffff0313097 in clang::ASTRecordReader::recordSwitchCaseID (this=0x7fffffff7ca8, SC=0x5022f0, ID=0) at ../../git/llvm-project/clang/include/clang/Serialization/ASTRecordReader.h:331
0x00007ffff0300ebc in clang::ASTStmtReader::VisitSwitchCase (this=0x7fffffff7c98, S=0x5022f0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:165
0x00007ffff0300f3f in clang::ASTStmtReader::VisitCaseStmt (this=0x7fffffff7c98, S=0x5022f0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:171
0x00007ffff031e768 in clang::StmtVisitorBase<std::add_pointer, clang::ASTStmtReader, void>::Visit (this=0x7fffffff7c98, S=0x5022f0) at tools/clang/include/clang/AST/StmtNodes.inc:561
0x00007ffff0312b7c in clang::ASTReader::ReadStmtFromStream (this=0x4d5710, F=...) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:3896
0x00007ffff030eb2e in clang::ASTReader::ReadStmt (this=0x4d5710, F=...) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:2696
0x00007ffff0312e2d in clang::ASTReader::ReadExpr (this=0x4d5710, F=...) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:2705
0x00007ffff01cbb10 in clang::ASTRecordReader::readExpr (this=0x7fffffff8450) at ../../git/llvm-project/clang/include/clang/Serialization/ASTRecordReader.h:131
0x00007ffff0293b23 in clang::ASTDeclReader::VisitVarDeclImpl (this=0x7fffffff8408, VD=0x5010d0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:1419
0x00007ffff02c40ad in clang::ASTDeclReader::VisitVarDecl (this=0x7fffffff8408, VD=0x5010d0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:371
0x00007ffff02be24c in clang::declvisitor::Base<std::add_pointer, clang::ASTDeclReader, void>::Visit (this=0x7fffffff8408, D=0x5010d0) at tools/clang/include/clang/AST/DeclNodes.inc:453
0x00007ffff028e5ca in clang::ASTDeclReader::Visit (this=0x7fffffff8408, D=0x5010d0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:519
0x00007ffff02b938f in clang::ASTReader::ReadDeclRecord (this=0x4d5710, ID=29) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:4044
0x00007ffff018c714 in clang::ASTReader::GetDecl (this=0x4d5710, ID=29) at ../../git/llvm-project/clang/lib/Serialization/ASTReader.cpp:7422
0x00007ffff021f9fa in clang::ASTReader::ReadDecl (this=0x4d5710, F=..., R=llvm::SmallVector of Size 3, Capacity 64 = {...}, I=@0x7fffffff9830: 3) at ../../git/llvm-project/clang/include/clang/Serialization/ASTReader.h:1863
0x00007ffff021f9a6 in clang::ASTRecordReader::readDecl (this=0x7fffffff9818) at ../../git/llvm-project/clang/include/clang/Serialization/ASTRecordReader.h:192
0x00007ffff0313d88 in clang::ASTStmtReader::readDecl (this=0x7fffffff9808) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:92
0x00007ffff0301d6a in clang::ASTStmtReader::VisitDeclStmt (this=0x7fffffff9808, S=0x5010b0) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:340
0x00007ffff031e078 in clang::StmtVisitorBase<std::add_pointer, clang::ASTStmtReader, void>::Visit (this=0x7fffffff9808, S=0x5010b0) at tools/clang/include/clang/AST/StmtNodes.inc:97
0x00007ffff0312b7c in clang::ASTReader::ReadStmtFromStream (this=0x4d5710, F=...) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:3896
0x00007ffff019a07c in clang::ASTReader::GetExternalDeclStmt (this=0x4d5710, Offset=107033) at ../../git/llvm-project/clang/lib/Serialization/ASTReader.cpp:7476

• 2)

clang::ASTReader::RecordSwitchCaseID (this=0x4d5710, SC=0x502440, ID=0) at ../../git/llvm-project/clang/lib/Serialization/ASTReader.cpp:9070
0x00007ffff0313097 in clang::ASTRecordReader::recordSwitchCaseID (this=0x7fffffff9818, SC=0x502440, ID=0) at ../../git/llvm-project/clang/include/clang/Serialization/ASTRecordReader.h:331
0x00007ffff0300ebc in clang::ASTStmtReader::VisitSwitchCase (this=0x7fffffff9808, S=0x502440) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:165
0x00007ffff030102f in clang::ASTStmtReader::VisitDefaultStmt (this=0x7fffffff9808, S=0x502440) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:182
0x00007ffff031e780 in clang::StmtVisitorBase<std::add_pointer, clang::ASTStmtReader, void>::Visit (this=0x7fffffff9808, S=0x502440) at tools/clang/include/clang/AST/StmtNodes.inc:567
0x00007ffff0312b7c in clang::ASTReader::ReadStmtFromStream (this=0x4d5710, F=...) at ../../git/llvm-project/clang/lib/Serialization/ASTReaderStmt.cpp:3896
0x00007ffff019a07c in clang::ASTReader::GetExternalDeclStmt (this=0x4d5710, Offset=107033) at ../../git/llvm-project/clang/lib/Serialization/ASTReader.cpp:7476

(In Bruno's modified example we always see the 2) call stack, meaning that the ids are cleared and everything is ok.)

My first attempt for the fix was to try to call ClearSwitchCaseIDs in VisitVarDecl, but that fired the other assertion in getSwitchCaseWithID for some other test cases.

The second attempt was to clear the SwitchCaseIDs in ASTStmtReader::VisitSwitchStmt and that indeed succeeded with the particular reproducer, but the original problem (reported by @vabridgers) was still crashing. Unfortunately, I could not create another minimal reproducer for this attempt, seems like there must be some different order of lazy loading of bodies to get the crash. So, I suspected there is something fundamentally wrong with the tracking of the SwitchCases and I came up with this patch.

[martong]

In D82940#2125183, @riccibruno wrote:

I'm not certain I understand, but I'm removing my LG while we figure it out. Can you expound a bit further?

Take a look at the AST dump of a small modification of the reduced test case which doesn't trigger the assertion:

void should_not_crash_with_switch_in_lambda() {
  switch (1)
  default:;
  auto f = [] {
    switch (1)
    default:;
  };
}

Before the serialization of LambdaExpr for the LambdaExpr *E, we have E->getBody() == E->getCallOperator()->getBody().
After serialization this is not true anymore because we are writing and reading the body directly when visiting the LambdaExpr
(the captures have the same problem).

The same is true in case of the simplest lambda example:

void foo() {   // line 11
    []{};
}

The AST that we got from the Parser has only one CompoundStmt node, but referenced in the CXXMethodDecl and added as a direct child to LambdaExpr:

`-LambdaExpr 0x5643235372c8 <line:12:5, col:8>
  |-CXXRecordDecl 0x564323536c98 <col:5> col:5 implicit class definition
  | |-CXXMethodDecl 0x564323536dd8 <col:6, col:8> col:5 constexpr operator() 'auto () const -> void' inline
  | | `-CompoundStmt 0x564323536e88 <col:7, col:8>
  `-CompoundStmt 0x564323536e88 <col:7, col:8>

After serialization and deserialization, the CompoundStmt is just copied, notice the different pointer values:

`-LambdaExpr 0x55a49f35f830 <line:12:5, col:8>
  |-CXXRecordDecl 0x55a49f35f860 <col:5> col:5 imported implicit <undeserialized declarations> class definition
  | |-CXXMethodDecl 0x55a49f35fac8 <col:6, col:8> col:5 imported constexpr operator() 'auto () const -> void' inline
  | | `-CompoundStmt 0x55a49f360110 <col:7, col:8>
  `-CompoundStmt 0x55a49f35f820 <col:7, col:8>

This suggests that we should prepare the ASTReader/ASTWriter to be able to handle references to Stmts similarly as it can already handle references to Decls (see ASTWriter::DeclIDs).
Does this sound as a good direction, should I move forward this way?

[riccibruno]

In D82940#2125939, @martong wrote:

In D82940#2125183, @riccibruno wrote:

I'm not certain I understand, but I'm removing my LG while we figure it out. Can you expound a bit further?

Take a look at the AST dump of a small modification of the reduced test case which doesn't trigger the assertion:

void should_not_crash_with_switch_in_lambda() {
  switch (1)
  default:;
  auto f = [] {
    switch (1)
    default:;
  };
}

Before the serialization of LambdaExpr for the LambdaExpr *E, we have E->getBody() == E->getCallOperator()->getBody().
After serialization this is not true anymore because we are writing and reading the body directly when visiting the LambdaExpr
(the captures have the same problem).

The same is true in case of the simplest lambda example:

void foo() {   // line 11
    []{};
}

The AST that we got from the Parser has only one CompoundStmt node, but referenced in the CXXMethodDecl and added as a direct child to LambdaExpr:

`-LambdaExpr 0x5643235372c8 <line:12:5, col:8>
  |-CXXRecordDecl 0x564323536c98 <col:5> col:5 implicit class definition
  | |-CXXMethodDecl 0x564323536dd8 <col:6, col:8> col:5 constexpr operator() 'auto () const -> void' inline
  | | `-CompoundStmt 0x564323536e88 <col:7, col:8>
  `-CompoundStmt 0x564323536e88 <col:7, col:8>

After serialization and deserialization, the CompoundStmt is just copied, notice the different pointer values:

`-LambdaExpr 0x55a49f35f830 <line:12:5, col:8>
  |-CXXRecordDecl 0x55a49f35f860 <col:5> col:5 imported implicit <undeserialized declarations> class definition
  | |-CXXMethodDecl 0x55a49f35fac8 <col:6, col:8> col:5 imported constexpr operator() 'auto () const -> void' inline
  | | `-CompoundStmt 0x55a49f360110 <col:7, col:8>
  `-CompoundStmt 0x55a49f35f820 <col:7, col:8>

This suggests that we should prepare the ASTReader/ASTWriter to be able to handle references to Stmts similarly as it can already handle references to Decls (see ASTWriter::DeclIDs).
Does this sound as a good direction, should I move forward this way?

Yes, I think that my patch just traded-off a bug for another bug. What about the following instead:

Initially zero the Stmt * for the body, then when the body is needed (either with getBody or with children)
populate it if needed from the call operator. This is what was done before with getBody.

This preserve the benefit of having a lazily deserialized body while being a simple fix.

Thoughts? Am I missing something?

If not I can cook-up a patch either tonight or tomorrow.

[riccibruno]

Yes, I think that my patch just traded-off a bug for another bug. What about the following instead:

Initially zero the Stmt * for the body, then when the body is needed (either with getBody or with children)
populate it if needed from the call operator. This is what was done before with getBody.

This preserve the benefit of having a lazily deserialized body while being a simple fix.

Thoughts? Am I missing something?

If not I can cook-up a patch either tonight or tomorrow.

D83009

[martong]

Abandoning for a better fix in D83009

clang/lib/Serialization/ASTReader.cpp

 }
 
 /// Resolve the offset of a statement into a statement.
 ///
 /// This operation will read a new statement from the external
 /// 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);
   if (llvm::Error Err = Loc.F->DeclsCursor.JumpToBit(Loc.Offset)) {
     Error(std::move(Err));
     return nullptr;
   }
   assert(NumCurrentElementsDeserializing == 0 &&

 /// preprocessor.
 IdentifierTable &ASTReader::getIdentifierTable() {
   return PP.getIdentifierTable();
 }
 
 /// 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;
 }
 
 /// Retrieve the switch-case statement with the given ID.
 SwitchCase *ASTReader::getSwitchCaseWithID(unsigned ID) {
   assert((*CurrSwitchCaseStmts)[ID] != nullptr && "No SwitchCase with this ID");
   return (*CurrSwitchCaseStmts)[ID];
 }

clang/lib/Serialization/ASTWriterDecl.cpp

void ASTWriter::WriteDecl(ASTContext &Context, Decl *D) {
 
   // Note declarations that should be deserialized eagerly so that we can add
   // them to a record in the AST file later.
   if (isRequiredDecl(D, Context, WritingModule))
     EagerlyDeserializedDecls.push_back(ID);
 }
 
 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()) {
     Optional<GVALinkage> Linkage;
     if (Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
       // When building a C++ Modules TS module interface unit, a strong
       // definition in the module interface is provided by the compilation of
       // that module interface unit, not by its users. (Inline functions are

clang/lib/Serialization/ASTWriterStmt.cpp

void ASTStmtWriter::VisitOMPTargetTeamsDistributeSimdDirective(
   Code = serialization::STMT_OMP_TARGET_TEAMS_DISTRIBUTE_SIMD_DIRECTIVE;
 }
 
 //===----------------------------------------------------------------------===//
 // ASTWriter Implementation
 //===----------------------------------------------------------------------===//
 
 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) {
   assert(SwitchCaseIDs.find(S) != SwitchCaseIDs.end() &&
          "SwitchCase hasn't been seen yet");
   return SwitchCaseIDs[S];
 }
 

clang/test/AST/ast-dump-lambda.cpp

template <typename... Ts> void test(Ts... a) {
   [&] { return c; };
   [b, &c] { return b + c; };
   [a..., x = 12] {};
   []() constexpr {};
   []() mutable {};
   []() 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
 // CHECK-NEXT:`-FunctionDecl {{.*}} <col:27, line:36:1> line:15:32{{( imported)?}} test 'void (Ts...)'
 // CHECK-NEXT:  |-ParmVarDecl {{.*}} <col:37, col:43> col:43{{( imported)?}} referenced a 'Ts...' pack
 // CHECK-NEXT:  `-CompoundStmt {{.*}} <col:46, line:36:1>
 // CHECK-NEXT:    |-DeclStmt {{.*}} <line:16:3, line:21:4>
 // CHECK-NEXT:    | `-CXXRecordDecl {{.*}} <line:16:3, line:21:3> line:16:10{{( imported)?}}{{( <undeserialized declarations>)?}} struct V definition