-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Serialization] Record whether the ODR is skipped (#82302)
Close #80570. In a0b6747, we skipped ODR checks for decls in GMF. Then it should be natural to skip storing the ODR values in BMI. Generally it should be fine as long as the writer and the reader keep consistent. However, the use of preamble in clangd shows the tricky part. For, ``` // test.cpp module; // any one off these is enough to crash clangd // #include <iostream> // #include <string_view> // #include <cmath> // #include <system_error> // #include <new> // #include <bit> // probably many more // only ok with libc++, not the system provided libstdc++ 13.2.1 // these are ok export module test; ``` clangd will store the headers as preamble to speedup the parsing and the preamble reuses the serialization techniques. (Generally we'd call the preamble as PCH. However it is not true strictly. I've tested the PCH wouldn't be problematic.) However, the tricky part is that the preamble is not modules. It literally serialiaze and deserialize things. So before clangd parsing the above test module, clangd will serialize the headers into the preamble. Note that there is no concept like GMF now. So the ODR bits are stored. However, when clangd parse the file actually, the decls from preamble are thought as in GMF literally, then hte ODR bits are skipped. Then mismatch happens. To solve the problem, this patch adds another bit for decls to record whether or not the ODR bits are skipped.
- Loading branch information
1 parent
c8fab88
commit 49775b1
Showing
5 changed files
with
154 additions
and
10 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
132 changes: 132 additions & 0 deletions
132
clang/unittests/Serialization/PreambleInNamedModulesTest.cpp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| //===- unittests/Serialization/PreambleInNamedModulesTest.cpp -------------===// | ||
| // | ||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
| // See https://llvm.org/LICENSE.txt for license information. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #include "clang/Frontend/CompilerInstance.h" | ||
| #include "clang/Frontend/CompilerInvocation.h" | ||
| #include "clang/Frontend/FrontendActions.h" | ||
| #include "clang/Frontend/PrecompiledPreamble.h" | ||
| #include "llvm/ADT/SmallString.h" | ||
| #include "llvm/Support/FileSystem.h" | ||
| #include "llvm/Support/raw_ostream.h" | ||
|
|
||
| #include "gtest/gtest.h" | ||
|
|
||
| using namespace llvm; | ||
| using namespace clang; | ||
|
|
||
| namespace { | ||
|
|
||
| class PreambleInNamedModulesTest : public ::testing::Test { | ||
| void SetUp() override { | ||
| ASSERT_FALSE(sys::fs::createUniqueDirectory("modules-test", TestDir)); | ||
| } | ||
|
|
||
| void TearDown() override { sys::fs::remove_directories(TestDir); } | ||
|
|
||
| public: | ||
| using PathType = SmallString<256>; | ||
|
|
||
| PathType TestDir; | ||
|
|
||
| void addFile(StringRef Path, StringRef Contents, PathType &AbsPath) { | ||
| ASSERT_FALSE(sys::path::is_absolute(Path)); | ||
|
|
||
| AbsPath = TestDir; | ||
| sys::path::append(AbsPath, Path); | ||
|
|
||
| ASSERT_FALSE( | ||
| sys::fs::create_directories(llvm::sys::path::parent_path(AbsPath))); | ||
|
|
||
| std::error_code EC; | ||
| llvm::raw_fd_ostream OS(AbsPath, EC); | ||
| ASSERT_FALSE(EC); | ||
| OS << Contents; | ||
| } | ||
|
|
||
| void addFile(StringRef Path, StringRef Contents) { | ||
| PathType UnusedAbsPath; | ||
| addFile(Path, Contents, UnusedAbsPath); | ||
| } | ||
| }; | ||
|
|
||
| // Testing that the use of Preamble in named modules can work basically. | ||
| // See https://github.com/llvm/llvm-project/issues/80570 | ||
| TEST_F(PreambleInNamedModulesTest, BasicTest) { | ||
| addFile("foo.h", R"cpp( | ||
| enum class E { | ||
| A, | ||
| B, | ||
| C, | ||
| D | ||
| }; | ||
| )cpp"); | ||
|
|
||
| PathType MainFilePath; | ||
| addFile("A.cppm", R"cpp( | ||
| module; | ||
| #include "foo.h" | ||
| export module A; | ||
| export using ::E; | ||
| )cpp", | ||
| MainFilePath); | ||
|
|
||
| IntrusiveRefCntPtr<DiagnosticsEngine> Diags = | ||
| CompilerInstance::createDiagnostics(new DiagnosticOptions()); | ||
| IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = | ||
| llvm::vfs::createPhysicalFileSystem(); | ||
|
|
||
| CreateInvocationOptions CIOpts; | ||
| CIOpts.Diags = Diags; | ||
| CIOpts.VFS = VFS; | ||
|
|
||
| const char *Args[] = {"clang++", "-std=c++20", "-working-directory", | ||
| TestDir.c_str(), MainFilePath.c_str()}; | ||
| std::shared_ptr<CompilerInvocation> Invocation = | ||
| createInvocation(Args, CIOpts); | ||
| ASSERT_TRUE(Invocation); | ||
|
|
||
| llvm::ErrorOr<std::unique_ptr<MemoryBuffer>> ContentsBuffer = | ||
| llvm::MemoryBuffer::getFile(MainFilePath, /*IsText=*/true); | ||
| EXPECT_TRUE(ContentsBuffer); | ||
| std::unique_ptr<MemoryBuffer> Buffer = std::move(*ContentsBuffer); | ||
|
|
||
| PreambleBounds Bounds = | ||
| ComputePreambleBounds(Invocation->getLangOpts(), *Buffer, 0); | ||
|
|
||
| PreambleCallbacks Callbacks; | ||
| llvm::ErrorOr<PrecompiledPreamble> BuiltPreamble = PrecompiledPreamble::Build( | ||
| *Invocation, Buffer.get(), Bounds, *Diags, VFS, | ||
| std::make_shared<PCHContainerOperations>(), | ||
| /*StoreInMemory=*/false, /*StoragePath=*/TestDir, Callbacks); | ||
|
|
||
| ASSERT_FALSE(Diags->hasErrorOccurred()); | ||
|
|
||
| EXPECT_TRUE(BuiltPreamble); | ||
| EXPECT_TRUE(BuiltPreamble->CanReuse(*Invocation, *Buffer, Bounds, *VFS)); | ||
| BuiltPreamble->OverridePreamble(*Invocation, VFS, Buffer.get()); | ||
|
|
||
| auto Clang = std::make_unique<CompilerInstance>( | ||
| std::make_shared<PCHContainerOperations>()); | ||
| Clang->setInvocation(std::move(Invocation)); | ||
| Clang->setDiagnostics(Diags.get()); | ||
|
|
||
| if (auto VFSWithRemapping = createVFSFromCompilerInvocation( | ||
| Clang->getInvocation(), Clang->getDiagnostics(), VFS)) | ||
| VFS = VFSWithRemapping; | ||
|
|
||
| Clang->createFileManager(VFS); | ||
| EXPECT_TRUE(Clang->createTarget()); | ||
|
|
||
| Buffer.release(); | ||
|
|
||
| SyntaxOnlyAction Action; | ||
| EXPECT_TRUE(Clang->ExecuteAction(Action)); | ||
| EXPECT_FALSE(Clang->getDiagnosticsPtr()->hasErrorOccurred()); | ||
| } | ||
|
|
||
| } // namespace |