ClangImporter: Use a separate CompilerInstance for canReadPCH #28125
Conversation
|
@swift-ci please test |
|
@swift-ci please smoke-test |
|
@swift-ci smoke test |
|
Build failed |
dexonsmith
force-pushed
the
clang-importer/separate-compiler-instance-for-can-read-pch
branch
from
6121bea to
d06b335
Compare
|
@swift-ci smoke test |
|
@swift-ci please smoke test |
|
Build failed |
dexonsmith
force-pushed
the
clang-importer/separate-compiler-instance-for-can-read-pch
branch
from
d06b335 to
930ee29
Compare
|
@swift-ci smoke-test |
|
@swift-ci smoke test |
|
@swift-ci please smoke test |
|
@swift-ci smoke test osx |
1 similar comment
|
@swift-ci smoke test osx |
|
@swift-ci please smoke-test osx |
|
@swift-ci smoke test osx |
|
@swift-ci smoke test |
|
@swift-ci smoke test osx |
|
@swift-ci test |
|
@swift-ci please test |
|
@swift-ci smoke test osx |
|
@swift-ci test osx |
|
Build failed |
dexonsmith
force-pushed
the
clang-importer/separate-compiler-instance-for-can-read-pch
branch
from
930ee29 to
b45a599
Compare
|
@swift-ci test |
|
@swift-ci smoke test |
|
@swift-ci please smoke test |
|
@swift-ci please test |
|
@swift-ci smoke test osx |
|
Build failed |
|
Build failed |
|
@swift-ci test osx |
|
@swift-ci test |
|
Build failed |
|
Build failed |
|
@swift-ci test macOS |
dexonsmith
force-pushed
the
clang-importer/separate-compiler-instance-for-can-read-pch
branch
from
b45a599 to
4fc9e97
Compare
|
@swift-ci smoke test |
|
@swift-ci please test |
|
@swift-ci please smoke test |
|
@brentdax @adrian-prantl, This is ready for review now. Let me know if you have any questions or concerns! |
ClangImporter::canReadPCH has been trying to reset the main CompilerInstance back to as if it hadn't run, but there's a long tail of potential memory corruption here. One example is that it's reusing HeaderSearch, but HeaderSearch contains a ModuleMap whose SourceLocations will point into the soon-to-be-released ASTContext on the stack. Instead, just use a separate CompilerInstnace, like the rest of the ClangImporter does when it needs a different ASTContext. This fixes a few potential memory corruption bugs, and it *may* be related to the persistent "missing submodule X" problems hitting Linux bots.
dexonsmith
force-pushed
the
clang-importer/separate-compiler-instance-for-can-read-pch
branch
from
4fc9e97 to
178d209
Compare
|
@swift-ci smoke test |
|
@swift-ci smoke test linux |
dexonsmith
deleted the
clang-importer/separate-compiler-instance-for-can-read-pch
branch
|
I think this change makes sense. I'm a little concerned that it could end up performing more work and thus regressing performance, so I've created a revert PR so I can test its performance, but I don't plan to merge the revert. |
Thanks Brent! |
ClangImporter::canReadPCH has been trying to reset the main
CompilerInstance back to as if it hadn't run, but there's a long tail of
potential memory corruption here. One example is that it's reusing
HeaderSearch, but HeaderSearch contains a ModuleMap whose
SourceLocations will point into the soon-to-be-released ASTContext on
the stack. Instead, just use a separate CompilerInstnace, like the rest
of the ClangImporter does when it needs a different ASTContext.
This fixes a few potential memory corruption bugs, and it may be
related to the persistent "missing submodule X" problems hitting Linux
bots.
Note: assigning to myself while I run tests. I'll add reviewers when ready.