Skip to content

Commit

Permalink
[ASTReader] Allow controlling separately whether validation should be…
Browse files Browse the repository at this point in the history
… disabled for a PCH vs a module file

This addresses an issue with how the PCH preable works, specifically:

1. When using a PCH/preamble the module hash changes and a different cache directory is used
2. When the preamble is used, PCH & PCM validation is disabled.

Due to combination of #1 and #2, reparsing with preamble enabled can end up loading a stale module file before a header change and using it without updating it because validation is disabled and it doesn’t check that the header has changed and the module file is out-of-date.

rdar://72611253

Differential Revision: https://reviews.llvm.org/D95159
  • Loading branch information
akyrtzi committed Jan 22, 2021
1 parent c042aff commit b0e8990
Show file tree
Hide file tree
Showing 17 changed files with 176 additions and 41 deletions.
3 changes: 2 additions & 1 deletion clang/include/clang/Driver/Options.td
Original file line number Diff line number Diff line change
Expand Up @@ -5121,7 +5121,8 @@ def fhalf_no_semantic_interposition : Flag<["-"], "fhalf-no-semantic-interpositi
MarshallingInfoFlag<LangOpts<"HalfNoSemanticInterposition">>;
def fno_validate_pch : Flag<["-"], "fno-validate-pch">,
HelpText<"Disable validation of precompiled headers">,
MarshallingInfoFlag<PreprocessorOpts<"DisablePCHValidation">>;
MarshallingInfoFlag<PreprocessorOpts<"DisablePCHOrModuleValidation">, "DisableValidationForModuleKind::None">,
Normalizer<"makeFlagToValueNormalizer(DisableValidationForModuleKind::All)">;
def fallow_pcm_with_errors : Flag<["-"], "fallow-pcm-with-compiler-errors">,
HelpText<"Accept a PCM file that was created with compiler errors">,
MarshallingInfoFlag<FrontendOpts<"AllowPCMWithCompilerErrors">>;
Expand Down
12 changes: 7 additions & 5 deletions clang/include/clang/Frontend/CompilerInstance.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ class Preprocessor;
class Sema;
class SourceManager;
class TargetInfo;
enum class DisableValidationForModuleKind;

/// CompilerInstance - Helper class for managing a single instance of the Clang
/// compiler.
Expand Down Expand Up @@ -659,16 +660,17 @@ class CompilerInstance : public ModuleLoader {

/// Create an external AST source to read a PCH file and attach it to the AST
/// context.
void createPCHExternalASTSource(StringRef Path, bool DisablePCHValidation,
bool AllowPCHWithCompilerErrors,
void *DeserializationListener,
bool OwnDeserializationListener);
void createPCHExternalASTSource(
StringRef Path, DisableValidationForModuleKind DisableValidation,
bool AllowPCHWithCompilerErrors, void *DeserializationListener,
bool OwnDeserializationListener);

/// Create an external AST source to read a PCH file.
///
/// \return - The new object on success, or null on failure.
static IntrusiveRefCntPtr<ASTReader> createPCHExternalASTSource(
StringRef Path, StringRef Sysroot, bool DisablePCHValidation,
StringRef Path, StringRef Sysroot,
DisableValidationForModuleKind DisableValidation,
bool AllowPCHWithCompilerErrors, Preprocessor &PP,
InMemoryModuleCache &ModuleCache, ASTContext &Context,
const PCHContainerReader &PCHContainerRdr,
Expand Down
26 changes: 23 additions & 3 deletions clang/include/clang/Lex/PreprocessorOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#ifndef LLVM_CLANG_LEX_PREPROCESSOROPTIONS_H_
#define LLVM_CLANG_LEX_PREPROCESSOROPTIONS_H_

#include "clang/Basic/BitmaskEnum.h"
#include "clang/Basic/LLVM.h"
#include "clang/Lex/PreprocessorExcludedConditionalDirectiveSkipMapping.h"
#include "llvm/ADT/StringRef.h"
Expand Down Expand Up @@ -40,6 +41,24 @@ enum ObjCXXARCStandardLibraryKind {
ARCXX_libstdcxx
};

/// Whether to disable the normal validation performed on precompiled
/// headers and module files when they are loaded.
enum class DisableValidationForModuleKind {
/// Perform validation, don't disable it.
None = 0,

/// Disable validation for a precompiled header and the modules it depends on.
PCH = 0x1,

/// Disable validation for module files.
Module = 0x2,

/// Disable validation for all kinds.
All = PCH | Module,

LLVM_MARK_AS_BITMASK_ENUM(Module)
};

/// PreprocessorOptions - This class is used for passing the various options
/// used in preprocessor initialization to InitializePreprocessor().
class PreprocessorOptions {
Expand Down Expand Up @@ -79,9 +98,10 @@ class PreprocessorOptions {
/// Headers that will be converted to chained PCHs in memory.
std::vector<std::string> ChainedIncludes;

/// When true, disables most of the normal validation performed on
/// precompiled headers.
bool DisablePCHValidation = false;
/// Whether to disable most of the normal validation performed on
/// precompiled headers and module files.
DisableValidationForModuleKind DisablePCHOrModuleValidation =
DisableValidationForModuleKind::None;

/// When true, a PCH with compiler errors will not be rejected.
bool AllowPCHWithCompilerErrors = false;
Expand Down
18 changes: 13 additions & 5 deletions clang/include/clang/Serialization/ASTReader.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "clang/Lex/ExternalPreprocessorSource.h"
#include "clang/Lex/HeaderSearch.h"
#include "clang/Lex/PreprocessingRecord.h"
#include "clang/Lex/PreprocessorOptions.h"
#include "clang/Sema/ExternalSemaSource.h"
#include "clang/Sema/IdentifierResolver.h"
#include "clang/Sema/Sema.h"
Expand Down Expand Up @@ -441,6 +442,9 @@ class ASTReader
/// imported from. For non-module AST types it should be invalid.
SourceLocation CurrentImportLoc;

/// The module kind that is currently deserializing.
Optional<ModuleKind> CurrentDeserializingModuleKind;

/// The global module index, if loaded.
std::unique_ptr<GlobalModuleIndex> GlobalIndex;

Expand Down Expand Up @@ -931,8 +935,8 @@ class ASTReader
std::string isysroot;

/// Whether to disable the normal validation performed on precompiled
/// headers when they are loaded.
bool DisableValidation;
/// headers and module files when they are loaded.
DisableValidationForModuleKind DisableValidationKind;

/// Whether to accept an AST file with compiler errors.
bool AllowASTWithCompilerErrors;
Expand Down Expand Up @@ -1215,6 +1219,8 @@ class ASTReader

llvm::DenseMap<const Decl *, bool> DefinitionSource;

bool shouldDisableValidationForFile(const serialization::ModuleFile &M) const;

/// Reads a statement from the specified cursor.
Stmt *ReadStmtFromStream(ModuleFile &F);

Expand Down Expand Up @@ -1473,9 +1479,9 @@ class ASTReader
/// user. This is only used with relocatable PCH files. If non-NULL,
/// a relocatable PCH file will use the default path "/".
///
/// \param DisableValidation If true, the AST reader will suppress most
/// \param DisableValidationKind If set, the AST reader will suppress most
/// of its regular consistency checking, allowing the use of precompiled
/// headers that cannot be determined to be compatible.
/// headers and module files that cannot be determined to be compatible.
///
/// \param AllowASTWithCompilerErrors If true, the AST reader will accept an
/// AST file the was created out of an AST with compiler errors,
Expand All @@ -1496,7 +1502,9 @@ class ASTReader
ASTReader(Preprocessor &PP, InMemoryModuleCache &ModuleCache,
ASTContext *Context, const PCHContainerReader &PCHContainerRdr,
ArrayRef<std::shared_ptr<ModuleFileExtension>> Extensions,
StringRef isysroot = "", bool DisableValidation = false,
StringRef isysroot = "",
DisableValidationForModuleKind DisableValidationKind =
DisableValidationForModuleKind::None,
bool AllowASTWithCompilerErrors = false,
bool AllowConfigurationMismatch = false,
bool ValidateSystemInputs = false,
Expand Down
5 changes: 3 additions & 2 deletions clang/lib/Frontend/ASTUnit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -809,9 +809,10 @@ std::unique_ptr<ASTUnit> ASTUnit::LoadFromASTFile(
PP.getIdentifierTable(), PP.getSelectorTable(),
PP.getBuiltinInfo());

bool disableValid = false;
DisableValidationForModuleKind disableValid =
DisableValidationForModuleKind::None;
if (::getenv("LIBCLANG_DISABLE_PCH_VALIDATION"))
disableValid = true;
disableValid = DisableValidationForModuleKind::All;
AST->Reader = new ASTReader(
PP, *AST->ModuleCache, AST->Ctx.get(), PCHContainerRdr, {},
/*isysroot=*/"",
Expand Down
11 changes: 6 additions & 5 deletions clang/lib/Frontend/ChainedIncludesSource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ createASTReader(CompilerInstance &CI, StringRef pchFile,
ASTDeserializationListener *deserialListener = nullptr) {
Preprocessor &PP = CI.getPreprocessor();
std::unique_ptr<ASTReader> Reader;
Reader.reset(new ASTReader(PP, CI.getModuleCache(), &CI.getASTContext(),
CI.getPCHContainerReader(),
/*Extensions=*/{},
/*isysroot=*/"", /*DisableValidation=*/true));
Reader.reset(new ASTReader(
PP, CI.getModuleCache(), &CI.getASTContext(), CI.getPCHContainerReader(),
/*Extensions=*/{},
/*isysroot=*/"", DisableValidationForModuleKind::PCH));
for (unsigned ti = 0; ti < bufNames.size(); ++ti) {
StringRef sr(bufNames[ti]);
Reader->addInMemoryBuffer(sr, std::move(MemBufs[ti]));
Expand Down Expand Up @@ -129,7 +129,8 @@ IntrusiveRefCntPtr<ExternalSemaSource> clang::createChainedIncludesSource(

CInvok->getPreprocessorOpts().ChainedIncludes.clear();
CInvok->getPreprocessorOpts().ImplicitPCHInclude.clear();
CInvok->getPreprocessorOpts().DisablePCHValidation = true;
CInvok->getPreprocessorOpts().DisablePCHOrModuleValidation =
DisableValidationForModuleKind::PCH;
CInvok->getPreprocessorOpts().Includes.clear();
CInvok->getPreprocessorOpts().MacroIncludes.clear();
CInvok->getPreprocessorOpts().Macros.clear();
Expand Down
15 changes: 9 additions & 6 deletions clang/lib/Frontend/CompilerInstance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -503,11 +503,12 @@ void CompilerInstance::createASTContext() {
// ExternalASTSource

void CompilerInstance::createPCHExternalASTSource(
StringRef Path, bool DisablePCHValidation, bool AllowPCHWithCompilerErrors,
void *DeserializationListener, bool OwnDeserializationListener) {
StringRef Path, DisableValidationForModuleKind DisableValidation,
bool AllowPCHWithCompilerErrors, void *DeserializationListener,
bool OwnDeserializationListener) {
bool Preamble = getPreprocessorOpts().PrecompiledPreambleBytes.first != 0;
TheASTReader = createPCHExternalASTSource(
Path, getHeaderSearchOpts().Sysroot, DisablePCHValidation,
Path, getHeaderSearchOpts().Sysroot, DisableValidation,
AllowPCHWithCompilerErrors, getPreprocessor(), getModuleCache(),
getASTContext(), getPCHContainerReader(),
getFrontendOpts().ModuleFileExtensions, DependencyCollectors,
Expand All @@ -516,7 +517,8 @@ void CompilerInstance::createPCHExternalASTSource(
}

IntrusiveRefCntPtr<ASTReader> CompilerInstance::createPCHExternalASTSource(
StringRef Path, StringRef Sysroot, bool DisablePCHValidation,
StringRef Path, StringRef Sysroot,
DisableValidationForModuleKind DisableValidation,
bool AllowPCHWithCompilerErrors, Preprocessor &PP,
InMemoryModuleCache &ModuleCache, ASTContext &Context,
const PCHContainerReader &PCHContainerRdr,
Expand All @@ -528,7 +530,7 @@ IntrusiveRefCntPtr<ASTReader> CompilerInstance::createPCHExternalASTSource(

IntrusiveRefCntPtr<ASTReader> Reader(new ASTReader(
PP, ModuleCache, &Context, PCHContainerRdr, Extensions,
Sysroot.empty() ? "" : Sysroot.data(), DisablePCHValidation,
Sysroot.empty() ? "" : Sysroot.data(), DisableValidation,
AllowPCHWithCompilerErrors, /*AllowConfigurationMismatch*/ false,
HSOpts.ModulesValidateSystemHeaders, HSOpts.ValidateASTInputFilesContent,
UseGlobalModuleIndex));
Expand Down Expand Up @@ -1497,7 +1499,8 @@ void CompilerInstance::createASTReader() {
TheASTReader = new ASTReader(
getPreprocessor(), getModuleCache(), &getASTContext(),
getPCHContainerReader(), getFrontendOpts().ModuleFileExtensions,
Sysroot.empty() ? "" : Sysroot.c_str(), PPOpts.DisablePCHValidation,
Sysroot.empty() ? "" : Sysroot.c_str(),
PPOpts.DisablePCHOrModuleValidation,
/*AllowASTWithCompilerErrors=*/FEOpts.AllowPCMWithCompilerErrors,
/*AllowConfigurationMismatch=*/false, HSOpts.ModulesValidateSystemHeaders,
HSOpts.ValidateASTInputFilesContent,
Expand Down
6 changes: 3 additions & 3 deletions clang/lib/Frontend/FrontendAction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -873,9 +873,9 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
if (!CI.getPreprocessorOpts().ImplicitPCHInclude.empty()) {
CI.createPCHExternalASTSource(
CI.getPreprocessorOpts().ImplicitPCHInclude,
CI.getPreprocessorOpts().DisablePCHValidation,
CI.getPreprocessorOpts().AllowPCHWithCompilerErrors, DeserialListener,
DeleteDeserialListener);
CI.getPreprocessorOpts().DisablePCHOrModuleValidation,
CI.getPreprocessorOpts().AllowPCHWithCompilerErrors,
DeserialListener, DeleteDeserialListener);
if (!CI.getASTContext().getExternalSource())
goto failure;
}
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Frontend/FrontendActions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ void VerifyPCHAction::ExecuteAction() {
CI.getPreprocessor(), CI.getModuleCache(), &CI.getASTContext(),
CI.getPCHContainerReader(), CI.getFrontendOpts().ModuleFileExtensions,
Sysroot.empty() ? "" : Sysroot.c_str(),
/*DisableValidation*/ false,
DisableValidationForModuleKind::None,
/*AllowASTWithCompilerErrors*/ false,
/*AllowConfigurationMismatch*/ true,
/*ValidateSystemInputs*/ true));
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/Frontend/PrecompiledPreamble.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,8 @@ void PrecompiledPreamble::configurePreamble(
PreprocessorOpts.PrecompiledPreambleBytes.first = Bounds.Size;
PreprocessorOpts.PrecompiledPreambleBytes.second =
Bounds.PreambleEndsAtStartOfLine;
PreprocessorOpts.DisablePCHValidation = true;
PreprocessorOpts.DisablePCHOrModuleValidation =
DisableValidationForModuleKind::PCH;

setupPreambleStorage(Storage, PreprocessorOpts, VFS);
}
Expand Down
47 changes: 40 additions & 7 deletions clang/lib/Serialization/ASTReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2211,6 +2211,29 @@ void ASTReader::resolvePendingMacro(IdentifierInfo *II,
PP.setLoadedMacroDirective(II, Earliest, Latest);
}

bool ASTReader::shouldDisableValidationForFile(
const serialization::ModuleFile &M) const {
if (DisableValidationKind == DisableValidationForModuleKind::None)
return false;

// If a PCH is loaded and validation is disabled for PCH then disable
// validation for the PCH and the modules it loads.
ModuleKind K = CurrentDeserializingModuleKind.getValueOr(M.Kind);

switch (K) {
case MK_MainFile:
case MK_Preamble:
case MK_PCH:
return bool(DisableValidationKind & DisableValidationForModuleKind::PCH);
case MK_ImplicitModule:
case MK_ExplicitModule:
case MK_PrebuiltModule:
return bool(DisableValidationKind & DisableValidationForModuleKind::Module);
}

return false;
}

ASTReader::InputFileInfo
ASTReader::readInputFileInfo(ModuleFile &F, unsigned ID) {
// Go find this input file.
Expand Down Expand Up @@ -2357,7 +2380,7 @@ InputFile ASTReader::getInputFile(ModuleFile &F, unsigned ID, bool Complain) {
auto HasInputFileChanged = [&]() {
if (StoredSize != File->getSize())
return ModificationType::Size;
if (!DisableValidation && StoredTime &&
if (!shouldDisableValidationForFile(F) && StoredTime &&
StoredTime != File->getModificationTime()) {
// In case the modification time changes but not the content,
// accept the cached file as legit.
Expand Down Expand Up @@ -2573,6 +2596,8 @@ ASTReader::ReadControlBlock(ModuleFile &F,
return Success;
};

bool DisableValidation = shouldDisableValidationForFile(F);

// Read all of the records and blocks in the control block.
RecordData Record;
unsigned NumInputs = 0;
Expand Down Expand Up @@ -2871,7 +2896,8 @@ ASTReader::ReadControlBlock(ModuleFile &F,
// If we're implicitly loading a module, the base directory can't
// change between the build and use.
// Don't emit module relocation error if we have -fno-validate-pch
if (!PP.getPreprocessorOpts().DisablePCHValidation &&
if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation &
DisableValidationForModuleKind::Module) &&
F.Kind != MK_ExplicitModule && F.Kind != MK_PrebuiltModule) {
auto BuildDir = PP.getFileManager().getDirectory(Blob);
if (!BuildDir || *BuildDir != M->Directory) {
Expand Down Expand Up @@ -3903,7 +3929,9 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
auto &Map = PP.getHeaderSearchInfo().getModuleMap();
const FileEntry *ModMap = M ? Map.getModuleMapFileForUniquing(M) : nullptr;
// Don't emit module relocation error if we have -fno-validate-pch
if (!PP.getPreprocessorOpts().DisablePCHValidation && !ModMap) {
if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation &
DisableValidationForModuleKind::Module) &&
!ModMap) {
if ((ClientLoadCapabilities & ARR_OutOfDate) == 0) {
if (auto ASTFE = M ? M->getASTFile() : None) {
// This module was defined by an imported (explicit) module.
Expand Down Expand Up @@ -4189,6 +4217,8 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
SmallVectorImpl<ImportedSubmodule> *Imported) {
llvm::SaveAndRestore<SourceLocation>
SetCurImportLocRAII(CurrentImportLoc, ImportLoc);
llvm::SaveAndRestore<Optional<ModuleKind>> SetCurModuleKindRAII(
CurrentDeserializingModuleKind, Type);

// Defer any pending actions until we get to the end of reading the AST file.
Deserializing AnASTFile(this);
Expand Down Expand Up @@ -4623,6 +4653,7 @@ ASTReader::readUnhashedControlBlock(ModuleFile &F, bool WasImportedBy,
PP.getHeaderSearchInfo().getHeaderSearchOpts();
bool AllowCompatibleConfigurationMismatch =
F.Kind == MK_ExplicitModule || F.Kind == MK_PrebuiltModule;
bool DisableValidation = shouldDisableValidationForFile(F);

ASTReadResult Result = readUnhashedControlBlockImpl(
&F, F.Data, ClientLoadCapabilities, AllowCompatibleConfigurationMismatch,
Expand Down Expand Up @@ -5514,7 +5545,8 @@ ASTReader::ReadSubmoduleBlock(ModuleFile &F, unsigned ClientLoadCapabilities) {
if (!ParentModule) {
if (const FileEntry *CurFile = CurrentModule->getASTFile()) {
// Don't emit module relocation error if we have -fno-validate-pch
if (!PP.getPreprocessorOpts().DisablePCHValidation &&
if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation &
DisableValidationForModuleKind::Module) &&
CurFile != F.File) {
Error(diag::err_module_file_conflict,
CurrentModule->getTopLevelModuleName(), CurFile->getName(),
Expand Down Expand Up @@ -11601,20 +11633,21 @@ ASTReader::ASTReader(Preprocessor &PP, InMemoryModuleCache &ModuleCache,
ASTContext *Context,
const PCHContainerReader &PCHContainerRdr,
ArrayRef<std::shared_ptr<ModuleFileExtension>> Extensions,
StringRef isysroot, bool DisableValidation,
StringRef isysroot,
DisableValidationForModuleKind DisableValidationKind,
bool AllowASTWithCompilerErrors,
bool AllowConfigurationMismatch, bool ValidateSystemInputs,
bool ValidateASTInputFilesContent, bool UseGlobalIndex,
std::unique_ptr<llvm::Timer> ReadTimer)
: Listener(DisableValidation
: Listener(bool(DisableValidationKind &DisableValidationForModuleKind::PCH)
? cast<ASTReaderListener>(new SimpleASTReaderListener(PP))
: cast<ASTReaderListener>(new PCHValidator(PP, *this))),
SourceMgr(PP.getSourceManager()), FileMgr(PP.getFileManager()),
PCHContainerRdr(PCHContainerRdr), Diags(PP.getDiagnostics()), PP(PP),
ContextObj(Context), ModuleMgr(PP.getFileManager(), ModuleCache,
PCHContainerRdr, PP.getHeaderSearchInfo()),
DummyIdResolver(PP), ReadTimer(std::move(ReadTimer)), isysroot(isysroot),
DisableValidation(DisableValidation),
DisableValidationKind(DisableValidationKind),
AllowASTWithCompilerErrors(AllowASTWithCompilerErrors),
AllowConfigurationMismatch(AllowConfigurationMismatch),
ValidateSystemInputs(ValidateSystemInputs),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
@interface I
- (void)call_me;
@end
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module mod {
header "head.h"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
@interface I
- (void)call_me_new;
@end
Loading

0 comments on commit b0e8990

Please sign in to comment.