Skip to content

Commit

Permalink
[C++20] [Modules] [Serialization] Check input file contents with opti…
Browse files Browse the repository at this point in the history
…on ForceCheckCXX20ModulesInputFiles enabled

With overriden input files, e,g,. the compiler get the file from an
in-memroy buffer, the compiler can't get correct modified time information
to indicate whehter the input files are changed or not. Then, the
semantics of ForceCheckCXX20ModulesInputFiles are broken.

In this patch, if both ForceCheckCXX20ModulesInputFiles and
ValidateASTInputFilesContent and enabled, the compiler will still check the
hash value of the contents even if their modification time is the same.
  • Loading branch information
ChuanqiXu9 committed Sep 14, 2023
1 parent 850e90c commit 45c1605
Show file tree
Hide file tree
Showing 3 changed files with 180 additions and 19 deletions.
54 changes: 35 additions & 19 deletions clang/lib/Serialization/ASTReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2489,6 +2489,32 @@ InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) {
std::optional<int64_t> Old = std::nullopt;
std::optional<int64_t> New = std::nullopt;
};
auto HasInputContentChanged = [&](Change OriginalChange) {
assert(ValidateASTInputFilesContent &&
"We should only check the content of the inputs with "
"ValidateASTInputFilesContent enabled.");

if (StoredContentHash == static_cast<uint64_t>(llvm::hash_code(-1)))
return OriginalChange;

auto MemBuffOrError = FileMgr.getBufferForFile(*File);
if (!MemBuffOrError) {
if (!Complain)
return OriginalChange;
std::string ErrorStr = "could not get buffer for file '";
ErrorStr += File->getName();
ErrorStr += "'";
Error(ErrorStr);
return OriginalChange;
}

// FIXME: hash_value is not guaranteed to be stable!
auto ContentHash = hash_value(MemBuffOrError.get()->getBuffer());
if (StoredContentHash == static_cast<uint64_t>(ContentHash))
return Change{Change::None};

return Change{Change::Content};
};
auto HasInputFileChanged = [&]() {
if (StoredSize != File->getSize())
return Change{Change::Size, StoredSize, File->getSize()};
Expand All @@ -2499,33 +2525,23 @@ InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) {

// In case the modification time changes but not the content,
// accept the cached file as legit.
if (ValidateASTInputFilesContent &&
StoredContentHash != static_cast<uint64_t>(llvm::hash_code(-1))) {
auto MemBuffOrError = FileMgr.getBufferForFile(*File);
if (!MemBuffOrError) {
if (!Complain)
return MTimeChange;
std::string ErrorStr = "could not get buffer for file '";
ErrorStr += File->getName();
ErrorStr += "'";
Error(ErrorStr);
return MTimeChange;
}

// FIXME: hash_value is not guaranteed to be stable!
auto ContentHash = hash_value(MemBuffOrError.get()->getBuffer());
if (StoredContentHash == static_cast<uint64_t>(ContentHash))
return Change{Change::None};
if (ValidateASTInputFilesContent)
return HasInputContentChanged(MTimeChange);

return Change{Change::Content};
}
return MTimeChange;
}
return Change{Change::None};
};

bool IsOutOfDate = false;
auto FileChange = SkipChecks ? Change{Change::None} : HasInputFileChanged();
// When ForceCheckCXX20ModulesInputFiles and ValidateASTInputFilesContent
// enabled, it is better to check the contents of the inputs. Since we can't
// get correct modified time information for inputs from overriden inputs.
if (HSOpts.ForceCheckCXX20ModulesInputFiles && ValidateASTInputFilesContent &&
F.StandardCXXModule && FileChange.Kind == Change::None)
FileChange = HasInputContentChanged(FileChange);

// For an overridden file, there is nothing to validate.
if (!Overridden && FileChange.Kind != Change::None) {
if (Complain && !Diags.isDiagnosticInFlight()) {
Expand Down
1 change: 1 addition & 0 deletions clang/unittests/Serialization/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ set(LLVM_LINK_COMPONENTS
)

add_clang_unittest(SerializationTests
ForceCheckFileInputTest.cpp
InMemoryModuleCacheTest.cpp
ModuleCacheTest.cpp
NoCommentsTest.cpp
Expand Down
144 changes: 144 additions & 0 deletions clang/unittests/Serialization/ForceCheckFileInputTest.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
//===- unittests/Serialization/ForceCheckFileInputTest.cpp - CI tests -----===//
//
// 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/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Basic/FileManager.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Frontend/CompilerInvocation.h"
#include "clang/Frontend/FrontendActions.h"
#include "clang/Frontend/Utils.h"
#include "clang/Lex/HeaderSearch.h"
#include "clang/Lex/PreprocessorOptions.h"
#include "clang/Serialization/ASTReader.h"
#include "clang/Tooling/Tooling.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 ForceCheckFileInputTest : public ::testing::Test {
void SetUp() override {
EXPECT_FALSE(sys::fs::createUniqueDirectory("modules-test", TestDir));
}

void TearDown() override { sys::fs::remove_directories(TestDir); }

public:
SmallString<256> TestDir;

void addFile(StringRef Path, StringRef Contents) {
EXPECT_FALSE(sys::path::is_absolute(Path));

SmallString<256> AbsPath(TestDir);
sys::path::append(AbsPath, Path);

EXPECT_FALSE(
sys::fs::create_directories(llvm::sys::path::parent_path(AbsPath)));

std::error_code EC;
llvm::raw_fd_ostream OS(AbsPath, EC);
EXPECT_FALSE(EC);
OS << Contents;
}
};

TEST_F(ForceCheckFileInputTest, ForceCheck) {
addFile("a.cppm", R"cpp(
export module a;
export int aa = 43;
)cpp");

std::string BMIPath = llvm::Twine(TestDir + "/a.pcm").str();

{
IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
CompilerInstance::createDiagnostics(new DiagnosticOptions());
CreateInvocationOptions CIOpts;
CIOpts.Diags = Diags;
CIOpts.VFS = llvm::vfs::createPhysicalFileSystem();

const char *Args[] = {
"clang++", "-std=c++20", "--precompile", "-working-directory",
TestDir.c_str(), "a.cppm", "-o", BMIPath.c_str()};
std::shared_ptr<CompilerInvocation> Invocation =
createInvocation(Args, CIOpts);
EXPECT_TRUE(Invocation);

auto Buf = CIOpts.VFS->getBufferForFile("a.cppm");
EXPECT_TRUE(Buf);

Invocation->getPreprocessorOpts().addRemappedFile("a.cppm", Buf->get());

Buf->release();

CompilerInstance Instance;
Instance.setDiagnostics(Diags.get());
Instance.setInvocation(Invocation);

if (auto VFSWithRemapping = createVFSFromCompilerInvocation(
Instance.getInvocation(), Instance.getDiagnostics(), CIOpts.VFS))
CIOpts.VFS = VFSWithRemapping;
Instance.createFileManager(CIOpts.VFS);

Instance.getHeaderSearchOpts().ValidateASTInputFilesContent = true;

GenerateModuleInterfaceAction Action;
EXPECT_TRUE(Instance.ExecuteAction(Action));
EXPECT_FALSE(Diags->hasErrorOccurred());
}

{
IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
CompilerInstance::createDiagnostics(new DiagnosticOptions());
CreateInvocationOptions CIOpts;
CIOpts.Diags = Diags;
CIOpts.VFS = llvm::vfs::createPhysicalFileSystem();

std::string BMIPath = llvm::Twine(TestDir + "/a.pcm").str();
const char *Args[] = {
"clang++", "-std=c++20", "--precompile", "-working-directory",
TestDir.c_str(), "a.cppm", "-o", BMIPath.c_str()};
std::shared_ptr<CompilerInvocation> Invocation =
createInvocation(Args, CIOpts);
EXPECT_TRUE(Invocation);

CompilerInstance Clang;

Clang.setInvocation(Invocation);
Clang.setDiagnostics(Diags.get());
FileManager *FM = Clang.createFileManager(CIOpts.VFS);
Clang.createSourceManager(*FM);

EXPECT_TRUE(Clang.createTarget());
Clang.createPreprocessor(TU_Complete);
Clang.getHeaderSearchOpts().ForceCheckCXX20ModulesInputFiles = true;
Clang.getHeaderSearchOpts().ValidateASTInputFilesContent = true;
Clang.createASTReader();

addFile("a.cppm", R"cpp(
export module a;
export int aa = 44;
)cpp");

auto ReadResult =
Clang.getASTReader()->ReadAST(BMIPath, serialization::MK_MainFile,
SourceLocation(), ASTReader::ARR_None);

// We shall be able to detect the content change here.
EXPECT_NE(ReadResult, ASTReader::Success);
}
}

} // anonymous namespace

0 comments on commit 45c1605

Please sign in to comment.