https://github.com/jansvoboda11 updated https://github.com/llvm/llvm-project/pull/68957
>From 4f60df88e1623733a64896ef332fd9a31e5b0e47 Mon Sep 17 00:00:00 2001 From: Jan Svoboda <jan_svob...@apple.com> Date: Thu, 12 Oct 2023 21:46:47 -0700 Subject: [PATCH 1/3] [clang][modules] Use file name as requested This prevents redefinition errors due to having multiple paths for the same module map. (rdar://24116019) Originally implemented and tested downstream by @bcardosolopes, I just made use of `FileEntryRef::getNameAsRequested()`. --- clang/lib/Lex/HeaderSearch.cpp | 2 +- clang/lib/Serialization/ASTWriter.cpp | 4 +-- .../Modules/Inputs/all-product-headers.yaml | 33 +++++++++++++++++++ clang/test/Modules/modulemap-collision.m | 15 +++++++++ 4 files changed, 51 insertions(+), 3 deletions(-) create mode 100644 clang/test/Modules/Inputs/all-product-headers.yaml create mode 100644 clang/test/Modules/modulemap-collision.m diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp index 4c8b64a374b47d5..cf1c0cc5284f316 100644 --- a/clang/lib/Lex/HeaderSearch.cpp +++ b/clang/lib/Lex/HeaderSearch.cpp @@ -1623,7 +1623,7 @@ bool HeaderSearch::findUsableModuleForHeader( ModuleMap::KnownHeader *SuggestedModule, bool IsSystemHeaderDir) { if (needModuleLookup(RequestingModule, SuggestedModule)) { // If there is a module that corresponds to this header, suggest it. - hasModuleMap(File.getName(), Root, IsSystemHeaderDir); + hasModuleMap(File.getNameAsRequested(), Root, IsSystemHeaderDir); return suggestModule(*this, File, RequestingModule, SuggestedModule); } return true; diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 27700c711d52fdd..0d216927d76260d 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -1384,10 +1384,10 @@ void ASTWriter::WriteControlBlock(Preprocessor &PP, ASTContext &Context, SmallVector<FileEntryRef, 1> ModMaps(AdditionalModMaps->begin(), AdditionalModMaps->end()); llvm::sort(ModMaps, [](FileEntryRef A, FileEntryRef B) { - return A.getName() < B.getName(); + return A.getNameAsRequested() < B.getNameAsRequested(); }); for (FileEntryRef F : ModMaps) - AddPath(F.getName(), Record); + AddPath(F.getNameAsRequested(), Record); } else { Record.push_back(0); } diff --git a/clang/test/Modules/Inputs/all-product-headers.yaml b/clang/test/Modules/Inputs/all-product-headers.yaml new file mode 100644 index 000000000000000..53d683f2ad2ecc0 --- /dev/null +++ b/clang/test/Modules/Inputs/all-product-headers.yaml @@ -0,0 +1,33 @@ +{ + 'version': 0, + 'case-sensitive': 'false', + 'roots': [ + { + 'type': 'directory', + 'name': "DUMMY_DIR/build/A.framework/PrivateHeaders" + 'contents': [ + { + 'type': 'file', + 'name': "A.h", + 'external-contents': "DUMMY_DIR/sources/A.h" + } + ] + }, + { + 'type': 'directory', + 'name': "DUMMY_DIR/build/A.framework/Modules" + 'contents': [ + { + 'type': 'file', + 'name': "module.modulemap", + 'external-contents': "DUMMY_DIR/build/module.modulemap" + }, + { + 'type': 'file', + 'name': "module.private.modulemap", + 'external-contents': "DUMMY_DIR/build/module.private.modulemap" + } + ] + } + ] +} diff --git a/clang/test/Modules/modulemap-collision.m b/clang/test/Modules/modulemap-collision.m new file mode 100644 index 000000000000000..5ada45da3dae191 --- /dev/null +++ b/clang/test/Modules/modulemap-collision.m @@ -0,0 +1,15 @@ +// REQUIRES: shell + +// RUN: rm -rf %t +// RUN: mkdir -p %t/sources %t/build +// RUN: echo "// A.h" > %t/sources/A.h +// RUN: echo "framework module A {}" > %t/sources/module.modulemap +// RUN: echo "framework module A.Private { umbrella header \"A.h\" }" > %t/sources/module.private.modulemap +// RUN: cp %t/sources/module.modulemap %t/build/module.modulemap +// RUN: cp %t/sources/module.private.modulemap %t/build/module.private.modulemap + +// RUN: sed -e "s:DUMMY_DIR:%t:g" %S/Inputs/all-product-headers.yaml > %t/build/all-product-headers.yaml +// RUN: %clang_cc1 -fsyntax-only -ivfsoverlay %t/build/all-product-headers.yaml -F%t/build -fmodules -fimplicit-module-maps -Wno-private-module -fmodules-cache-path=%t/cache -x objective-c %s -verify + +// expected-no-diagnostics +#import <A/A.h> >From 68a9858b1c4c8b2aaee4ebf6a72a9d3c081912fe Mon Sep 17 00:00:00 2001 From: Jan Svoboda <jan_svob...@apple.com> Date: Fri, 13 Oct 2023 17:23:47 -0700 Subject: [PATCH 2/3] [clang] Remove `PointerLikeTypeTraits<FileEntryRef>` --- clang/include/clang/ARCMigrate/FileRemapper.h | 4 +- clang/include/clang/Basic/FileEntry.h | 31 ----------- clang/include/clang/Basic/Module.h | 12 ++-- .../clang/Frontend/VerifyDiagnosticConsumer.h | 9 +-- clang/include/clang/Lex/ModuleMap.h | 4 +- clang/lib/ARCMigrate/FileRemapper.cpp | 55 +++++++++---------- clang/lib/Basic/Module.cpp | 8 +-- clang/lib/Lex/ModuleMap.cpp | 5 +- clang/lib/Serialization/ASTReader.cpp | 2 +- 9 files changed, 49 insertions(+), 81 deletions(-) diff --git a/clang/include/clang/ARCMigrate/FileRemapper.h b/clang/include/clang/ARCMigrate/FileRemapper.h index ceb3e67b5035272..afcee363516a211 100644 --- a/clang/include/clang/ARCMigrate/FileRemapper.h +++ b/clang/include/clang/ARCMigrate/FileRemapper.h @@ -12,10 +12,10 @@ #include "clang/Basic/FileEntry.h" #include "clang/Basic/LLVM.h" #include "llvm/ADT/DenseMap.h" -#include "llvm/ADT/PointerUnion.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" #include <memory> +#include <variant> namespace llvm { class MemoryBuffer; @@ -33,7 +33,7 @@ class FileRemapper { // FIXME: Reuse the same FileManager for multiple ASTContexts. std::unique_ptr<FileManager> FileMgr; - typedef llvm::PointerUnion<FileEntryRef, llvm::MemoryBuffer *> Target; + using Target = std::variant<FileEntryRef, llvm::MemoryBuffer *>; using MappingsTy = llvm::DenseMap<FileEntryRef, Target>; MappingsTy FromToMappings; diff --git a/clang/include/clang/Basic/FileEntry.h b/clang/include/clang/Basic/FileEntry.h index 23237a9326f84b6..bc6546373548841 100644 --- a/clang/include/clang/Basic/FileEntry.h +++ b/clang/include/clang/Basic/FileEntry.h @@ -235,37 +235,6 @@ static_assert(std::is_trivially_copyable<OptionalFileEntryRef>::value, namespace llvm { -template <> struct PointerLikeTypeTraits<clang::FileEntryRef> { - static inline void *getAsVoidPointer(clang::FileEntryRef File) { - return const_cast<clang::FileEntryRef::MapEntry *>(&File.getMapEntry()); - } - - static inline clang::FileEntryRef getFromVoidPointer(void *Ptr) { - return clang::FileEntryRef( - *reinterpret_cast<const clang::FileEntryRef::MapEntry *>(Ptr)); - } - - static constexpr int NumLowBitsAvailable = PointerLikeTypeTraits< - const clang::FileEntryRef::MapEntry *>::NumLowBitsAvailable; -}; - -template <> struct PointerLikeTypeTraits<clang::OptionalFileEntryRef> { - static inline void *getAsVoidPointer(clang::OptionalFileEntryRef File) { - if (!File) - return nullptr; - return PointerLikeTypeTraits<clang::FileEntryRef>::getAsVoidPointer(*File); - } - - static inline clang::OptionalFileEntryRef getFromVoidPointer(void *Ptr) { - if (!Ptr) - return std::nullopt; - return PointerLikeTypeTraits<clang::FileEntryRef>::getFromVoidPointer(Ptr); - } - - static constexpr int NumLowBitsAvailable = - PointerLikeTypeTraits<clang::FileEntryRef>::NumLowBitsAvailable; -}; - /// Specialisation of DenseMapInfo for FileEntryRef. template <> struct DenseMapInfo<clang::FileEntryRef> { static inline clang::FileEntryRef getEmptyKey() { diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h index 676fd372493a3aa..6a7423938bdb8fa 100644 --- a/clang/include/clang/Basic/Module.h +++ b/clang/include/clang/Basic/Module.h @@ -35,6 +35,7 @@ #include <optional> #include <string> #include <utility> +#include <variant> #include <vector> namespace llvm { @@ -162,7 +163,7 @@ class alignas(8) Module { std::string PresumedModuleMapFile; /// The umbrella header or directory. - llvm::PointerUnion<FileEntryRef, DirectoryEntryRef> Umbrella; + std::variant<std::monostate, FileEntryRef, DirectoryEntryRef> Umbrella; /// The module signature. ASTFileSignature Signature; @@ -665,18 +666,17 @@ class alignas(8) Module { /// Retrieve the umbrella directory as written. std::optional<DirectoryName> getUmbrellaDirAsWritten() const { - if (Umbrella && Umbrella.is<DirectoryEntryRef>()) + if (const auto *Dir = std::get_if<DirectoryEntryRef>(&Umbrella)) return DirectoryName{UmbrellaAsWritten, - UmbrellaRelativeToRootModuleDirectory, - Umbrella.get<DirectoryEntryRef>()}; + UmbrellaRelativeToRootModuleDirectory, *Dir}; return std::nullopt; } /// Retrieve the umbrella header as written. std::optional<Header> getUmbrellaHeaderAsWritten() const { - if (Umbrella && Umbrella.is<FileEntryRef>()) + if (const auto *Hdr = std::get_if<FileEntryRef>(&Umbrella)) return Header{UmbrellaAsWritten, UmbrellaRelativeToRootModuleDirectory, - Umbrella.get<FileEntryRef>()}; + *Hdr}; return std::nullopt; } diff --git a/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h b/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h index e0f3570e5b2bc6e..500a7e11ab9ac2a 100644 --- a/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h +++ b/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h @@ -278,14 +278,15 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer, // These facilities are used for validation in debug builds. class UnparsedFileStatus { - llvm::PointerIntPair<OptionalFileEntryRef, 1, bool> Data; + OptionalFileEntryRef File; + bool FoundDirectives; public: UnparsedFileStatus(OptionalFileEntryRef File, bool FoundDirectives) - : Data(File, FoundDirectives) {} + : File(File), FoundDirectives(FoundDirectives) {} - OptionalFileEntryRef getFile() const { return Data.getPointer(); } - bool foundDirectives() const { return Data.getInt(); } + OptionalFileEntryRef getFile() const { return File; } + bool foundDirectives() const { return FoundDirectives; } }; using ParsedFilesMap = llvm::DenseMap<FileID, const FileEntry *>; diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h index fc49742ad4af2c1..054d4211b9c6d41 100644 --- a/clang/include/clang/Lex/ModuleMap.h +++ b/clang/include/clang/Lex/ModuleMap.h @@ -20,8 +20,8 @@ #include "clang/Basic/SourceLocation.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/PointerIntPair.h" -#include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" @@ -194,7 +194,7 @@ class ModuleMap { } }; - using AdditionalModMapsSet = llvm::SmallPtrSet<FileEntryRef, 1>; + using AdditionalModMapsSet = llvm::DenseSet<FileEntryRef>; private: friend class ModuleMapParser; diff --git a/clang/lib/ARCMigrate/FileRemapper.cpp b/clang/lib/ARCMigrate/FileRemapper.cpp index bd8317e0a24515f..7abc862ceecc237 100644 --- a/clang/lib/ARCMigrate/FileRemapper.cpp +++ b/clang/lib/ARCMigrate/FileRemapper.cpp @@ -134,9 +134,8 @@ bool FileRemapper::flushToFile(StringRef outputPath, DiagnosticsEngine &Diag) { infoOut << origPath << '\n'; infoOut << (uint64_t)origFE.getModificationTime() << '\n'; - if (I->second.is<FileEntryRef>()) { - auto FE = I->second.get<FileEntryRef>(); - SmallString<200> newPath = StringRef(FE.getName()); + if (const auto *FE = std::get_if<FileEntryRef>(&I->second)) { + SmallString<200> newPath = StringRef(FE->getName()); fs::make_absolute(newPath); infoOut << newPath << '\n'; } else { @@ -150,7 +149,7 @@ bool FileRemapper::flushToFile(StringRef outputPath, DiagnosticsEngine &Diag) { return report("Could not create file: " + tempPath.str(), Diag); llvm::raw_fd_ostream newOut(fd, /*shouldClose=*/true); - llvm::MemoryBuffer *mem = I->second.get<llvm::MemoryBuffer *>(); + llvm::MemoryBuffer *mem = std::get<llvm::MemoryBuffer *>(I->second); newOut.write(mem->getBufferStart(), mem->getBufferSize()); newOut.close(); @@ -173,7 +172,7 @@ bool FileRemapper::overwriteOriginal(DiagnosticsEngine &Diag, for (MappingsTy::iterator I = FromToMappings.begin(), E = FromToMappings.end(); I != E; ++I) { FileEntryRef origFE = I->first; - assert(I->second.is<llvm::MemoryBuffer *>()); + assert(std::holds_alternative<llvm::MemoryBuffer *>(I->second)); if (!fs::exists(origFE.getName())) return report(StringRef("File does not exist: ") + origFE.getName(), Diag); @@ -183,7 +182,7 @@ bool FileRemapper::overwriteOriginal(DiagnosticsEngine &Diag, if (EC) return report(EC.message(), Diag); - llvm::MemoryBuffer *mem = I->second.get<llvm::MemoryBuffer *>(); + llvm::MemoryBuffer *mem = std::get<llvm::MemoryBuffer *>(I->second); Out.write(mem->getBufferStart(), mem->getBufferSize()); Out.close(); } @@ -197,25 +196,23 @@ void FileRemapper::forEachMapping( llvm::function_ref<void(StringRef, const llvm::MemoryBufferRef &)> CaptureBuffer) const { for (auto &Mapping : FromToMappings) { - if (Mapping.second.is<FileEntryRef>()) { - auto FE = Mapping.second.get<FileEntryRef>(); - CaptureFile(Mapping.first.getName(), FE.getName()); + if (const auto *FE = std::get_if<FileEntryRef>(&Mapping.second)) { + CaptureFile(Mapping.first.getName(), FE->getName()); continue; } CaptureBuffer( Mapping.first.getName(), - Mapping.second.get<llvm::MemoryBuffer *>()->getMemBufferRef()); + std::get<llvm::MemoryBuffer *>(Mapping.second)->getMemBufferRef()); } } void FileRemapper::applyMappings(PreprocessorOptions &PPOpts) const { for (MappingsTy::const_iterator I = FromToMappings.begin(), E = FromToMappings.end(); I != E; ++I) { - if (I->second.is<FileEntryRef>()) { - auto FE = I->second.get<FileEntryRef>(); - PPOpts.addRemappedFile(I->first.getName(), FE.getName()); + if (const auto *FE = std::get_if<FileEntryRef>(&I->second)) { + PPOpts.addRemappedFile(I->first.getName(), FE->getName()); } else { - llvm::MemoryBuffer *mem = I->second.get<llvm::MemoryBuffer *>(); + llvm::MemoryBuffer *mem = std::get<llvm::MemoryBuffer *>(I->second); PPOpts.addRemappedFile(I->first.getName(), mem); } } @@ -230,18 +227,20 @@ void FileRemapper::remap(StringRef filePath, remap(*File, std::move(memBuf)); } -void FileRemapper::remap(FileEntryRef file, - std::unique_ptr<llvm::MemoryBuffer> memBuf) { - Target &targ = FromToMappings[file]; - resetTarget(targ); - targ = memBuf.release(); +void FileRemapper::remap(FileEntryRef File, + std::unique_ptr<llvm::MemoryBuffer> MemBuf) { + auto [It, New] = FromToMappings.insert({File, nullptr}); + if (!New) + resetTarget(It->second); + It->second = MemBuf.release(); } -void FileRemapper::remap(FileEntryRef file, FileEntryRef newfile) { - Target &targ = FromToMappings[file]; - resetTarget(targ); - targ = newfile; - ToFromMappings.insert({newfile, file}); +void FileRemapper::remap(FileEntryRef File, FileEntryRef NewFile) { + auto [It, New] = FromToMappings.insert({File, nullptr}); + if (!New) + resetTarget(It->second); + It->second = NewFile; + ToFromMappings.insert({NewFile, File}); } OptionalFileEntryRef FileRemapper::getOriginalFile(StringRef filePath) { @@ -259,13 +258,11 @@ OptionalFileEntryRef FileRemapper::getOriginalFile(StringRef filePath) { } void FileRemapper::resetTarget(Target &targ) { - if (!targ) - return; - - if (llvm::MemoryBuffer *oldmem = targ.dyn_cast<llvm::MemoryBuffer *>()) { + if (std::holds_alternative<llvm::MemoryBuffer *>(targ)) { + llvm::MemoryBuffer *oldmem = std::get<llvm::MemoryBuffer *>(targ); delete oldmem; } else { - FileEntryRef toFE = targ.get<FileEntryRef>(); + FileEntryRef toFE = std::get<FileEntryRef>(targ); ToFromMappings.erase(toFE); } } diff --git a/clang/lib/Basic/Module.cpp b/clang/lib/Basic/Module.cpp index 7879a11179b6d4b..cc2e5be98d32d34 100644 --- a/clang/lib/Basic/Module.cpp +++ b/clang/lib/Basic/Module.cpp @@ -265,10 +265,10 @@ bool Module::fullModuleNameIs(ArrayRef<StringRef> nameParts) const { } OptionalDirectoryEntryRef Module::getEffectiveUmbrellaDir() const { - if (Umbrella && Umbrella.is<FileEntryRef>()) - return Umbrella.get<FileEntryRef>().getDir(); - if (Umbrella && Umbrella.is<DirectoryEntryRef>()) - return Umbrella.get<DirectoryEntryRef>(); + if (const auto *Hdr = std::get_if<FileEntryRef>(&Umbrella)) + return Hdr->getDir(); + if (const auto *Dir = std::get_if<DirectoryEntryRef>(&Umbrella)) + return *Dir; return std::nullopt; } diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp index f65a5f145c04395..7fd92bff8048429 100644 --- a/clang/lib/Lex/ModuleMap.cpp +++ b/clang/lib/Lex/ModuleMap.cpp @@ -2426,7 +2426,8 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken, Header.Kind = Map.headerRoleToKind(Role); // Check whether we already have an umbrella. - if (Header.IsUmbrella && ActiveModule->Umbrella) { + if (Header.IsUmbrella && + !std::holds_alternative<std::monostate>(ActiveModule->Umbrella)) { Diags.Report(Header.FileNameLoc, diag::err_mmap_umbrella_clash) << ActiveModule->getFullModuleName(); HadError = true; @@ -2523,7 +2524,7 @@ void ModuleMapParser::parseUmbrellaDirDecl(SourceLocation UmbrellaLoc) { SourceLocation DirNameLoc = consumeToken(); // Check whether we already have an umbrella. - if (ActiveModule->Umbrella) { + if (!std::holds_alternative<std::monostate>(ActiveModule->Umbrella)) { Diags.Report(DirNameLoc, diag::err_mmap_umbrella_clash) << ActiveModule->getFullModuleName(); HadError = true; diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 9ea8c8eacaa931b..26b048db9a7b5c4 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -4156,7 +4156,7 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F, return OutOfDate; } - llvm::SmallPtrSet<FileEntryRef, 1> AdditionalStoredMaps; + ModuleMap::AdditionalModMapsSet AdditionalStoredMaps; for (unsigned I = 0, N = Record[Idx++]; I < N; ++I) { // FIXME: we should use input files rather than storing names. std::string Filename = ReadPath(F, Record, Idx); >From 797e7b959a3f2931e396fb47d8800fdc798292d7 Mon Sep 17 00:00:00 2001 From: Jan Svoboda <jan_svob...@apple.com> Date: Fri, 13 Oct 2023 17:24:10 -0700 Subject: [PATCH 3/3] [clang][modules] Undo unnecessary `FileEntryRef::getNameAsRequested()` --- clang/lib/Serialization/ASTWriter.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 0d216927d76260d..27700c711d52fdd 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -1384,10 +1384,10 @@ void ASTWriter::WriteControlBlock(Preprocessor &PP, ASTContext &Context, SmallVector<FileEntryRef, 1> ModMaps(AdditionalModMaps->begin(), AdditionalModMaps->end()); llvm::sort(ModMaps, [](FileEntryRef A, FileEntryRef B) { - return A.getNameAsRequested() < B.getNameAsRequested(); + return A.getName() < B.getName(); }); for (FileEntryRef F : ModMaps) - AddPath(F.getNameAsRequested(), Record); + AddPath(F.getName(), Record); } else { Record.push_back(0); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits