Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ void ExpandModularHeadersPPCallbacks::InclusionDirective(
if (ModuleImported) {
serialization::ModuleFile *MF =
Compiler.getASTReader()->getModuleManager().lookup(
*SuggestedModule->getASTFile());
*SuggestedModule->getASTFileKey());
handleModuleFile(MF);
}
parseToLocation(DirectiveLoc);
Expand Down
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,7 @@ libclang
- Visit constraints of `auto` type to properly visit concept usages (#GH166580)
- Visit switch initializer statements (https://bugs.kde.org/show_bug.cgi?id=415537#c2)
- Fix crash in clang_getBinaryOperatorKindSpelling and clang_getUnaryOperatorKindSpelling
- The clang_Module_getASTFile API is deprecated and now always returns nullptr
Comment thread
benlangmuir marked this conversation as resolved.

Code Completion
---------------
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang-c/Index.h
Original file line number Diff line number Diff line change
Expand Up @@ -4651,6 +4651,8 @@ CINDEX_LINKAGE CXModule clang_getModuleForFile(CXTranslationUnit, CXFile);
* \param Module a module object.
*
* \returns the module file where the provided module object came from.
*
* @deprecated: module files are longer guaranteed to be loaded from a CXFile
*/
CINDEX_LINKAGE CXFile clang_Module_getASTFile(CXModule Module);

Expand Down
29 changes: 20 additions & 9 deletions clang/include/clang/Basic/Module.h
Original file line number Diff line number Diff line change
Expand Up @@ -365,9 +365,10 @@ class alignas(8) Module {
/// \c SubModules vector at which that submodule resides.
mutable llvm::StringMap<unsigned> SubModuleIndex;

/// The AST file if this is a top-level module which has a
/// The AST file name and key if this is a top-level module which has a
/// corresponding serialized AST file, or null otherwise.
OptionalFileEntryRef ASTFile;
std::optional<ModuleFileName> ASTFileName;
std::optional<ModuleFileKey> ASTFileKey;

/// The top-level headers associated with this module.
llvm::SmallSetVector<FileEntryRef, 2> TopHeaders;
Expand Down Expand Up @@ -840,15 +841,25 @@ class alignas(8) Module {
return getTopLevelModule()->Name;
}

/// The serialized AST file for this module, if one was created.
OptionalFileEntryRef getASTFile() const {
return getTopLevelModule()->ASTFile;
/// The serialized AST file name for this module, if one was created.
const ModuleFileName *getASTFileName() const {
const Module *TopLevel = getTopLevelModule();
return TopLevel->ASTFileName ? &*TopLevel->ASTFileName : nullptr;
}

/// Set the serialized AST file for the top-level module of this module.
void setASTFile(OptionalFileEntryRef File) {
assert((!getASTFile() || getASTFile() == File) && "file path changed");
getTopLevelModule()->ASTFile = File;
/// The serialized AST file key for this module, if one was created.
const ModuleFileKey *getASTFileKey() const {
const Module *TopLevel = getTopLevelModule();
return TopLevel->ASTFileKey ? &*TopLevel->ASTFileKey : nullptr;
}

/// Set the serialized module file for the top-level module of this module.
void setASTFileNameAndKey(ModuleFileName NewName, ModuleFileKey NewKey) {
assert((!getASTFileName() && !getASTFileKey()) ||
*getASTFileKey() == NewKey && "file path changed");
Module *TopLevel = getTopLevelModule();
TopLevel->ASTFileName = NewName;
TopLevel->ASTFileKey = NewKey;
}

/// Retrieve the umbrella directory as written.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ struct PrebuiltModuleDep {

explicit PrebuiltModuleDep(const Module *M)
: ModuleName(M->getTopLevelModuleName()),
PCMFile(M->getASTFile()->getName()),
PCMFile(M->getASTFileName()->str()),
ModuleMapFile(M->PresumedModuleMapFile) {}
};

Expand Down
4 changes: 2 additions & 2 deletions clang/lib/Basic/ASTSourceDescriptor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ ASTSourceDescriptor::ASTSourceDescriptor(Module &M)
: Signature(M.Signature), ClangModule(&M) {
if (M.Directory)
Path = M.Directory->getName();
if (auto File = M.getASTFile())
ASTFile = File->getName();
if (auto FileKey = M.getASTFileName())
ASTFile = FileKey->str();
}

std::string ASTSourceDescriptor::getModuleName() const {
Expand Down
6 changes: 3 additions & 3 deletions clang/lib/DependencyScanning/ModuleDepCollector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,7 @@ ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
// -fmodule-name is used to compile a translation unit that imports this
// module. In that case it can be skipped. The appropriate header
// dependencies will still be reported as expected.
if (!M->getASTFile())
if (!M->getASTFileKey())
return {};

// If this module has been handled already, just return its ID.
Expand Down Expand Up @@ -716,7 +716,7 @@ ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {

serialization::ModuleFile *MF =
MDC.ScanInstance.getASTReader()->getModuleManager().lookup(
*M->getASTFile());
*M->getASTFileKey());

llvm::SmallString<256> Storage;
MD.FileDepsBaseDir =
Expand Down Expand Up @@ -938,7 +938,7 @@ bool ModuleDepCollector::isPrebuiltModule(const Module *M) {
if (PrebuiltModuleFileIt == PrebuiltModuleFiles.end())
return false;
assert("Prebuilt module came from the expected AST file" &&
PrebuiltModuleFileIt->second == M->getASTFile()->getName());
PrebuiltModuleFileIt->second == M->getASTFileName()->str());
return true;
}

Expand Down
10 changes: 4 additions & 6 deletions clang/lib/Frontend/CompilerInstance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1883,10 +1883,9 @@ ModuleLoadResult CompilerInstance::findOrCompileModuleAndReadAST(
M = HS.lookupModule(ModuleName, ImportLoc, true, !IsInclusionDirective);

// Check whether M refers to the file in the prebuilt module path.
if (M && M->getASTFile())
if (auto ModuleFile = FileMgr->getOptionalFileRef(ModuleFilename))
if (*ModuleFile == M->getASTFile())
return M;
if (M && M->getASTFileKey() &&
*M->getASTFileKey() == ModuleFilename.makeKey(*FileMgr))
return M;

getDiagnostics().Report(ModuleNameLoc, diag::err_module_prebuilt)
<< ModuleName;
Expand Down Expand Up @@ -2295,8 +2294,7 @@ GlobalModuleIndex *CompilerInstance::loadGlobalModuleIndex(
for (ModuleMap::module_iterator I = MMap.module_begin(),
E = MMap.module_end(); I != E; ++I) {
Module *TheModule = I->second;
OptionalFileEntryRef Entry = TheModule->getASTFile();
if (!Entry) {
if (!TheModule->getASTFileKey()) {
SmallVector<IdentifierLoc, 2> Path;
Path.emplace_back(TriggerLoc,
getPreprocessor().getIdentifierInfo(TheModule->Name));
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/Index/IndexingAction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,8 @@ static void indexPreprocessorModuleMacros(Preprocessor &PP,
if (M.second.getLatest() == nullptr) {
for (auto *MM : PP.getLeafModuleMacros(M.first)) {
auto *OwningMod = MM->getOwningModule();
if (OwningMod && OwningMod->getASTFile() == Mod.File) {
if (OwningMod && OwningMod->getASTFileKey() &&
*OwningMod->getASTFileKey() == Mod.FileKey) {
if (auto *MI = MM->getMacroInfo()) {
indexPreprocessorMacro(M.first, MI, MacroDirective::MD_Define,
MI->getDefinitionLoc(), DataConsumer);
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/Sema/SemaModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -386,9 +386,9 @@ Sema::ActOnModuleDecl(SourceLocation StartLoc, SourceLocation ModuleLoc,
Diag(Path[0].getLoc(), diag::err_module_redefinition) << ModuleName;
if (M->DefinitionLoc.isValid())
Diag(M->DefinitionLoc, diag::note_prev_module_definition);
else if (OptionalFileEntryRef FE = M->getASTFile())
else if (const ModuleFileName *FileName = M->getASTFileName())
Diag(M->DefinitionLoc, diag::note_prev_module_definition_from_ast_file)
<< FE->getName();
<< *FileName;
Mod = M;
break;
}
Expand Down
17 changes: 9 additions & 8 deletions clang/lib/Serialization/ASTReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4652,10 +4652,10 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
<< F.ModuleName << F.BaseDirectory << M->Directory->getName();

if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities)) {
if (auto ASTFE = M ? M->getASTFile() : std::nullopt) {
if (auto ASTFileName = M ? M->getASTFileName() : nullptr) {
// This module was defined by an imported (explicit) module.
Diag(diag::err_module_file_conflict) << F.ModuleName << F.FileName
<< ASTFE->getName();
Diag(diag::err_module_file_conflict)
<< F.ModuleName << F.FileName << *ASTFileName;
// TODO: Add a note with the module map paths if they differ.
} else {
// This module was built with a different module map.
Expand Down Expand Up @@ -6339,15 +6339,16 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
"too many submodules");

if (!ParentModule) {
if (OptionalFileEntryRef CurFile = CurrentModule->getASTFile()) {
if (const ModuleFileKey *CurFileKey = CurrentModule->getASTFileKey()) {
// Don't emit module relocation error if we have -fno-validate-pch
if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation &
DisableValidationForModuleKind::Module)) {
assert(CurFile != F.File && "ModuleManager did not de-duplicate");
assert(*CurFileKey != F.FileKey &&

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CurFileKey is used only here.

"ModuleManager did not de-duplicate");

Diag(diag::err_module_file_conflict)
<< CurrentModule->getTopLevelModuleName() << CurFile->getName()
<< F.File.getName();
<< CurrentModule->getTopLevelModuleName()
<< *CurrentModule->getASTFileName() << F.FileName;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static analysis flagged this as a potential nullptr deref. Is it safe to assume here getASTFileName() does not return a nullptr? In some places in this change, you check it and others not, but the assumption is not asserted nor is it apparent why.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key and name can only be set together. The fact getASTFileKey() is set here means the name cannot be null. Maybe worth an assert, but it is correct AFAICT.


auto CurModMapFile =
ModMap.getContainingModuleMapFile(CurrentModule);
Expand All @@ -6361,7 +6362,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
}

F.DidReadTopLevelSubmodule = true;
CurrentModule->setASTFile(F.File);
CurrentModule->setASTFileNameAndKey(F.FileName, F.FileKey);
CurrentModule->PresumedModuleMapFile = F.ModuleMapPath;
}

Expand Down
8 changes: 3 additions & 5 deletions clang/lib/Serialization/ModuleManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ ModuleFile *ModuleManager::lookupByFileName(StringRef Name) const {

ModuleFile *ModuleManager::lookupByModuleName(StringRef Name) const {
if (const Module *Mod = HeaderSearchInfo.getModuleMap().findModule(Name))
if (OptionalFileEntryRef File = Mod->getASTFile())
return lookup(*File);
if (const ModuleFileName *FileName = Mod->getASTFileName())
return lookupByFileName(*FileName);

return nullptr;
}
Expand Down Expand Up @@ -299,10 +299,8 @@ void ModuleManager::removeModules(ModuleIterator First) {
}

// Delete the modules.
for (ModuleIterator victim = First; victim != Last; ++victim) {
Modules.erase(ModuleFileKey(victim->File));
for (ModuleIterator victim = First; victim != Last; ++victim)
Modules.erase(victim->FileKey);
}

Chain.erase(Chain.begin() + (First - begin()), Chain.end());
}
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Index/annotate-module.m
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,6 @@
// RUN: c-index-test -cursor-at=%s:3:11 %s -fmodules-cache-path=%t.cache -fmodules -F %S/../Modules/Inputs \
// RUN: | FileCheck %s -check-prefix=CHECK-CURSOR

// CHECK-CURSOR: 3:1 ModuleImport=DependsOnModule:3:1 (Definition) Extent=[3:1 - 3:24] Spelling=DependsOnModule ([3:9 - 3:24]) ModuleName=DependsOnModule ({{.*}}DependsOnModule-{{[^.]*}}.pcm) system=0 Headers(2):
// CHECK-CURSOR: 3:1 ModuleImport=DependsOnModule:3:1 (Definition) Extent=[3:1 - 3:24] Spelling=DependsOnModule ([3:9 - 3:24]) ModuleName=DependsOnModule system=0 Headers(2):
// CHECK-CURSOR-NEXT: {{.*}}other.h
// CHECK-CURSOR-NEXT: {{.*}}DependsOnModule.h
2 changes: 1 addition & 1 deletion clang/test/Modules/DebugInfoSubmoduleImport.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
// RUN: %s -emit-llvm -debugger-tuning=lldb -o - | FileCheck %s
//
// RUN: %clang_cc1 -fmodules -fmodule-format=obj -debug-info-kind=limited -dwarf-ext-refs \
// RUN: -fimplicit-module-maps -x c -fmodules-cache-path=%t -I %S/Inputs \
// RUN: -fimplicit-module-maps -x c -fmodules-cache-path=%t/cache -I %S/Inputs \
// RUN: -fmodules-local-submodule-visibility \
// RUN: %s -emit-llvm -debugger-tuning=lldb -o - | FileCheck %s
#include "DebugSubmoduleA.h"
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Modules/DebugInfoTransitiveImport.m
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// UNSUPPORTED: target={{.*}}-zos{{.*}}, target={{.*}}-aix{{.*}}
// RUN: rm -rf %t
// RUN: %clang_cc1 -fmodules -fmodule-format=obj -debug-info-kind=limited -dwarf-ext-refs \
// RUN: -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs \
// RUN: -fimplicit-module-maps -fmodules-cache-path=%t/cache -I %S/Inputs \
// RUN: %s -mllvm -debug-only=pchcontainer -debugger-tuning=lldb 2>&1 | FileCheck %s
// REQUIRES: asserts

Expand Down
9 changes: 2 additions & 7 deletions clang/tools/c-index-test/c-index-test.c
Original file line number Diff line number Diff line change
Expand Up @@ -3099,19 +3099,14 @@ static void inspect_print_cursor(CXCursor Cursor) {

{
CXModule mod = clang_Cursor_getModule(Cursor);
CXFile astFile;
CXString name, astFilename;
CXString name;
unsigned i, numHeaders;
if (mod) {
astFile = clang_Module_getASTFile(mod);
astFilename = clang_getFileName(astFile);
name = clang_Module_getFullName(mod);
numHeaders = clang_Module_getNumTopLevelHeaders(TU, mod);
printf(" ModuleName=%s (%s) system=%d Headers(%d):",
clang_getCString(name), clang_getCString(astFilename),
printf(" ModuleName=%s system=%d Headers(%d):", clang_getCString(name),
clang_Module_isSystem(mod), numHeaders);
clang_disposeString(name);
clang_disposeString(astFilename);
for (i = 0; i < numHeaders; ++i) {
CXFile file = clang_Module_getTopLevelHeader(TU, mod, i);
CXString filename = clang_getFileName(file);
Expand Down
7 changes: 1 addition & 6 deletions clang/tools/libclang/CIndex.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9396,12 +9396,7 @@ CXModule clang_getModuleForFile(CXTranslationUnit TU, CXFile File) {
return Header.getModule();
}

CXFile clang_Module_getASTFile(CXModule CXMod) {
if (!CXMod)
return nullptr;
Module *Mod = static_cast<Module *>(CXMod);
return cxfile::makeCXFile(Mod->getASTFile());
}
CXFile clang_Module_getASTFile(CXModule CXMod) { return nullptr; }

CXModule clang_Module_getParent(CXModule CXMod) {
if (!CXMod)
Expand Down
6 changes: 5 additions & 1 deletion clang/tools/libclang/CXIndexDataConsumer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,11 @@ void CXIndexDataConsumer::importedModule(const ImportDecl *ImportD) {
if (SrcMod->getTopLevelModule() == Mod->getTopLevelModule())
return;

OptionalFileEntryRef FE = Mod->getASTFile();
OptionalFileEntryRef FE;
if (auto ASTFileName = Mod->getASTFileName()) {
FileManager &FileMgr = Ctx->getSourceManager().getFileManager();
FE = FileMgr.getOptionalFileRef(*ASTFileName);
}
CXIdxImportedASTFileInfo Info = {cxfile::makeCXFile(FE), Mod,
getIndexLoc(ImportD->getLocation()),
ImportD->isImplicit()};
Expand Down
Loading