Page MenuHomePhabricator

Correctly set CompilingPCH in PrecompilePreambleAction.
ClosedPublic

Authored by adamcz on Aug 7 2020, 9:01 AM.

Details

Summary

This fixes a crash bug in clangd when used with modules. ASTWriter would
end up writing references to submodules into the PCH file, but upon
reading the submodules would not exists and
HeaderFileInfoTrait::ReadData would crash.

Diff Detail

Event Timeline

adamcz created this revision.Aug 7 2020, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2020, 9:01 AM
adamcz requested review of this revision.Aug 7 2020, 9:01 AM
adamcz updated this revision to Diff 283922.Aug 7 2020, 9:03 AM

This time the fix is included ;-)

Regarding tests, it feels like we can also test this in ASTUnitTests which is directly in clang, as it is also using PrecompiledPreamble::Build. What about moving the test there instead?

clang-tools-extra/clangd/unittests/ModulesTests.cpp
25

nit: indent the string literals by at least 4 spaces. (that's what other tests look like, same below)

29

this one is not needed right?

clang/lib/Frontend/PrecompiledPreamble.cpp
423

anything operating on CompilerInstance before and including the callbacks invoked here will see a broken LangOtps. Why not set it here (and maybe assert in the override, or just not do anything at all)

adamcz updated this revision to Diff 284329.Aug 10 2020, 5:33 AM
adamcz marked 3 inline comments as done.

addressed review comments

clang-tools-extra/clangd/unittests/ModulesTests.cpp
29

I suppose not.

clang/lib/Frontend/PrecompiledPreamble.cpp
423

I think we should at least assert() on this, so others don't waste as much time as I did looking for bugs like this. PrecompilePreambleAction is not usable with CompilingPCH = false.

I'm a bit conflicted on setting CompilingPCH outside of PrecompilePreambleAction, it makes it seem like you can change it somehow and it makes that Action class not self-contained (it requires that external bit that sets the options), but with assert it should be fine and it's essentially an internal detail of PrecompilePreamble class anyway.

kadircet accepted this revision.Aug 10 2020, 6:09 AM

thanks, lgtm!

clang/unittests/Frontend/ASTUnitTest.cpp
144

nit: ASSRET_TRUE(AU);

146

ASSERT_TRUE again

This revision is now accepted and ready to land.Aug 10 2020, 6:09 AM
adamcz updated this revision to Diff 284365.Aug 10 2020, 7:43 AM
adamcz marked an inline comment as done.

more review comments

clang/unittests/Frontend/ASTUnitTest.cpp
146

You can 't ASSERT_TRUE() on llvm::Expected, since operator bool() is not const. No idea why, but that's why it's assert()

adamcz added inline comments.Aug 10 2020, 7:46 AM
clang/unittests/Frontend/ASTUnitTest.cpp
146

Nevermind, I do know why - because it changes the "checked" status, that makes sense. I guess we could make that mutable, but that seems out of scope for this change.

adamcz updated this revision to Diff 284390.Aug 10 2020, 8:30 AM

assert() -> ASSERT_TRUE

This revision was landed with ongoing or failed builds.Aug 10 2020, 8:50 AM
This revision was automatically updated to reflect the committed changes.