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

Wed Jul 1 07:00:20 PDT 2020

martong added a comment.

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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82940/new/

https://reviews.llvm.org/D82940