Skip to content

Commit 1afb313

Browse files
jansvoboda11jansvoboda11
authored
[clang][modules] Use file name as requested (llvm#68957)
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()`.
1 parent cb62f67 commit 1afb313

File tree

12 files changed

+98
-82
lines changed

12 files changed

+98
-82
lines changed

‎clang/include/clang/ARCMigrate/FileRemapper.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@
1212
#include "clang/Basic/FileEntry.h"
1313
#include "clang/Basic/LLVM.h"
1414
#include "llvm/ADT/DenseMap.h"
15-
#include "llvm/ADT/PointerUnion.h"
1615
#include "llvm/ADT/STLExtras.h"
1716
#include "llvm/ADT/StringRef.h"
1817
#include <memory>
18+
#include <variant>
1919

2020
namespace llvm {
2121
class MemoryBuffer;
@@ -33,7 +33,7 @@ class FileRemapper {
3333
// FIXME: Reuse the same FileManager for multiple ASTContexts.
3434
std::unique_ptr<FileManager> FileMgr;
3535

36-
typedef llvm::PointerUnion<FileEntryRef, llvm::MemoryBuffer *> Target;
36+
using Target = std::variant<FileEntryRef, llvm::MemoryBuffer *>;
3737
using MappingsTy = llvm::DenseMap<FileEntryRef, Target>;
3838
MappingsTy FromToMappings;
3939

‎clang/include/clang/Basic/FileEntry.h

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -235,37 +235,6 @@ static_assert(std::is_trivially_copyable<OptionalFileEntryRef>::value,
235235

236236
namespace llvm {
237237

238-
template <> struct PointerLikeTypeTraits<clang::FileEntryRef> {
239-
static inline void *getAsVoidPointer(clang::FileEntryRef File) {
240-
return const_cast<clang::FileEntryRef::MapEntry *>(&File.getMapEntry());
241-
}
242-
243-
static inline clang::FileEntryRef getFromVoidPointer(void *Ptr) {
244-
return clang::FileEntryRef(
245-
*reinterpret_cast<const clang::FileEntryRef::MapEntry *>(Ptr));
246-
}
247-
248-
static constexpr int NumLowBitsAvailable = PointerLikeTypeTraits<
249-
const clang::FileEntryRef::MapEntry *>::NumLowBitsAvailable;
250-
};
251-
252-
template <> struct PointerLikeTypeTraits<clang::OptionalFileEntryRef> {
253-
static inline void *getAsVoidPointer(clang::OptionalFileEntryRef File) {
254-
if (!File)
255-
return nullptr;
256-
return PointerLikeTypeTraits<clang::FileEntryRef>::getAsVoidPointer(*File);
257-
}
258-
259-
static inline clang::OptionalFileEntryRef getFromVoidPointer(void *Ptr) {
260-
if (!Ptr)
261-
return std::nullopt;
262-
return PointerLikeTypeTraits<clang::FileEntryRef>::getFromVoidPointer(Ptr);
263-
}
264-
265-
static constexpr int NumLowBitsAvailable =
266-
PointerLikeTypeTraits<clang::FileEntryRef>::NumLowBitsAvailable;
267-
};
268-
269238
/// Specialisation of DenseMapInfo for FileEntryRef.
270239
template <> struct DenseMapInfo<clang::FileEntryRef> {
271240
static inline clang::FileEntryRef getEmptyKey() {

‎clang/include/clang/Basic/Module.h

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include <optional>
3636
#include <string>
3737
#include <utility>
38+
#include <variant>
3839
#include <vector>
3940

4041
namespace llvm {
@@ -162,7 +163,7 @@ class alignas(8) Module {
162163
std::string PresumedModuleMapFile;
163164

164165
/// The umbrella header or directory.
165-
llvm::PointerUnion<FileEntryRef, DirectoryEntryRef> Umbrella;
166+
std::variant<std::monostate, FileEntryRef, DirectoryEntryRef> Umbrella;
166167

167168
/// The module signature.
168169
ASTFileSignature Signature;
@@ -665,18 +666,17 @@ class alignas(8) Module {
665666

666667
/// Retrieve the umbrella directory as written.
667668
std::optional<DirectoryName> getUmbrellaDirAsWritten() const {
668-
if (Umbrella && Umbrella.is<DirectoryEntryRef>())
669+
if (const auto *Dir = std::get_if<DirectoryEntryRef>(&Umbrella))
669670
return DirectoryName{UmbrellaAsWritten,
670-
UmbrellaRelativeToRootModuleDirectory,
671-
Umbrella.get<DirectoryEntryRef>()};
671+
UmbrellaRelativeToRootModuleDirectory, *Dir};
672672
return std::nullopt;
673673
}
674674

675675
/// Retrieve the umbrella header as written.
676676
std::optional<Header> getUmbrellaHeaderAsWritten() const {
677-
if (Umbrella && Umbrella.is<FileEntryRef>())
677+
if (const auto *Hdr = std::get_if<FileEntryRef>(&Umbrella))
678678
return Header{UmbrellaAsWritten, UmbrellaRelativeToRootModuleDirectory,
679-
Umbrella.get<FileEntryRef>()};
679+
*Hdr};
680680
return std::nullopt;
681681
}
682682

‎clang/include/clang/Frontend/VerifyDiagnosticConsumer.h

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -278,14 +278,15 @@ class VerifyDiagnosticConsumer: public DiagnosticConsumer,
278278

279279
// These facilities are used for validation in debug builds.
280280
class UnparsedFileStatus {
281-
llvm::PointerIntPair<OptionalFileEntryRef, 1, bool> Data;
281+
OptionalFileEntryRef File;
282+
bool FoundDirectives;
282283

283284
public:
284285
UnparsedFileStatus(OptionalFileEntryRef File, bool FoundDirectives)
285-
: Data(File, FoundDirectives) {}
286+
: File(File), FoundDirectives(FoundDirectives) {}
286287

287-
OptionalFileEntryRef getFile() const { return Data.getPointer(); }
288-
bool foundDirectives() const { return Data.getInt(); }
288+
OptionalFileEntryRef getFile() const { return File; }
289+
bool foundDirectives() const { return FoundDirectives; }
289290
};
290291

291292
using ParsedFilesMap = llvm::DenseMap<FileID, const FileEntry *>;

‎clang/include/clang/Lex/ModuleMap.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@
2020
#include "clang/Basic/SourceLocation.h"
2121
#include "llvm/ADT/ArrayRef.h"
2222
#include "llvm/ADT/DenseMap.h"
23+
#include "llvm/ADT/DenseSet.h"
2324
#include "llvm/ADT/PointerIntPair.h"
24-
#include "llvm/ADT/SmallPtrSet.h"
2525
#include "llvm/ADT/SmallVector.h"
2626
#include "llvm/ADT/StringMap.h"
2727
#include "llvm/ADT/StringRef.h"
@@ -194,7 +194,7 @@ class ModuleMap {
194194
}
195195
};
196196

197-
using AdditionalModMapsSet = llvm::SmallPtrSet<FileEntryRef, 1>;
197+
using AdditionalModMapsSet = llvm::DenseSet<FileEntryRef>;
198198

199199
private:
200200
friend class ModuleMapParser;

‎clang/lib/ARCMigrate/FileRemapper.cpp

Lines changed: 26 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -134,9 +134,8 @@ bool FileRemapper::flushToFile(StringRef outputPath, DiagnosticsEngine &Diag) {
134134
infoOut << origPath << '\n';
135135
infoOut << (uint64_t)origFE.getModificationTime() << '\n';
136136

137-
if (I->second.is<FileEntryRef>()) {
138-
auto FE = I->second.get<FileEntryRef>();
139-
SmallString<200> newPath = StringRef(FE.getName());
137+
if (const auto *FE = std::get_if<FileEntryRef>(&I->second)) {
138+
SmallString<200> newPath = StringRef(FE->getName());
140139
fs::make_absolute(newPath);
141140
infoOut << newPath << '\n';
142141
} else {
@@ -150,7 +149,7 @@ bool FileRemapper::flushToFile(StringRef outputPath, DiagnosticsEngine &Diag) {
150149
return report("Could not create file: " + tempPath.str(), Diag);
151150

152151
llvm::raw_fd_ostream newOut(fd, /*shouldClose=*/true);
153-
llvm::MemoryBuffer *mem = I->second.get<llvm::MemoryBuffer *>();
152+
llvm::MemoryBuffer *mem = std::get<llvm::MemoryBuffer *>(I->second);
154153
newOut.write(mem->getBufferStart(), mem->getBufferSize());
155154
newOut.close();
156155

@@ -173,7 +172,7 @@ bool FileRemapper::overwriteOriginal(DiagnosticsEngine &Diag,
173172
for (MappingsTy::iterator
174173
I = FromToMappings.begin(), E = FromToMappings.end(); I != E; ++I) {
175174
FileEntryRef origFE = I->first;
176-
assert(I->second.is<llvm::MemoryBuffer *>());
175+
assert(std::holds_alternative<llvm::MemoryBuffer *>(I->second));
177176
if (!fs::exists(origFE.getName()))
178177
return report(StringRef("File does not exist: ") + origFE.getName(),
179178
Diag);
@@ -183,7 +182,7 @@ bool FileRemapper::overwriteOriginal(DiagnosticsEngine &Diag,
183182
if (EC)
184183
return report(EC.message(), Diag);
185184

186-
llvm::MemoryBuffer *mem = I->second.get<llvm::MemoryBuffer *>();
185+
llvm::MemoryBuffer *mem = std::get<llvm::MemoryBuffer *>(I->second);
187186
Out.write(mem->getBufferStart(), mem->getBufferSize());
188187
Out.close();
189188
}
@@ -197,25 +196,23 @@ void FileRemapper::forEachMapping(
197196
llvm::function_ref<void(StringRef, const llvm::MemoryBufferRef &)>
198197
CaptureBuffer) const {
199198
for (auto &Mapping : FromToMappings) {
200-
if (Mapping.second.is<FileEntryRef>()) {
201-
auto FE = Mapping.second.get<FileEntryRef>();
202-
CaptureFile(Mapping.first.getName(), FE.getName());
199+
if (const auto *FE = std::get_if<FileEntryRef>(&Mapping.second)) {
200+
CaptureFile(Mapping.first.getName(), FE->getName());
203201
continue;
204202
}
205203
CaptureBuffer(
206204
Mapping.first.getName(),
207-
Mapping.second.get<llvm::MemoryBuffer *>()->getMemBufferRef());
205+
std::get<llvm::MemoryBuffer *>(Mapping.second)->getMemBufferRef());
208206
}
209207
}
210208

211209
void FileRemapper::applyMappings(PreprocessorOptions &PPOpts) const {
212210
for (MappingsTy::const_iterator
213211
I = FromToMappings.begin(), E = FromToMappings.end(); I != E; ++I) {
214-
if (I->second.is<FileEntryRef>()) {
215-
auto FE = I->second.get<FileEntryRef>();
216-
PPOpts.addRemappedFile(I->first.getName(), FE.getName());
212+
if (const auto *FE = std::get_if<FileEntryRef>(&I->second)) {
213+
PPOpts.addRemappedFile(I->first.getName(), FE->getName());
217214
} else {
218-
llvm::MemoryBuffer *mem = I->second.get<llvm::MemoryBuffer *>();
215+
llvm::MemoryBuffer *mem = std::get<llvm::MemoryBuffer *>(I->second);
219216
PPOpts.addRemappedFile(I->first.getName(), mem);
220217
}
221218
}
@@ -230,18 +227,20 @@ void FileRemapper::remap(StringRef filePath,
230227
remap(*File, std::move(memBuf));
231228
}
232229

233-
void FileRemapper::remap(FileEntryRef file,
234-
std::unique_ptr<llvm::MemoryBuffer> memBuf) {
235-
Target &targ = FromToMappings[file];
236-
resetTarget(targ);
237-
targ = memBuf.release();
230+
void FileRemapper::remap(FileEntryRef File,
231+
std::unique_ptr<llvm::MemoryBuffer> MemBuf) {
232+
auto [It, New] = FromToMappings.insert({File, nullptr});
233+
if (!New)
234+
resetTarget(It->second);
235+
It->second = MemBuf.release();
238236
}
239237

240-
void FileRemapper::remap(FileEntryRef file, FileEntryRef newfile) {
241-
Target &targ = FromToMappings[file];
242-
resetTarget(targ);
243-
targ = newfile;
244-
ToFromMappings.insert({newfile, file});
238+
void FileRemapper::remap(FileEntryRef File, FileEntryRef NewFile) {
239+
auto [It, New] = FromToMappings.insert({File, nullptr});
240+
if (!New)
241+
resetTarget(It->second);
242+
It->second = NewFile;
243+
ToFromMappings.insert({NewFile, File});
245244
}
246245

247246
OptionalFileEntryRef FileRemapper::getOriginalFile(StringRef filePath) {
@@ -259,13 +258,11 @@ OptionalFileEntryRef FileRemapper::getOriginalFile(StringRef filePath) {
259258
}
260259

261260
void FileRemapper::resetTarget(Target &targ) {
262-
if (!targ)
263-
return;
264-
265-
if (llvm::MemoryBuffer *oldmem = targ.dyn_cast<llvm::MemoryBuffer *>()) {
261+
if (std::holds_alternative<llvm::MemoryBuffer *>(targ)) {
262+
llvm::MemoryBuffer *oldmem = std::get<llvm::MemoryBuffer *>(targ);
266263
delete oldmem;
267264
} else {
268-
FileEntryRef toFE = targ.get<FileEntryRef>();
265+
FileEntryRef toFE = std::get<FileEntryRef>(targ);
269266
ToFromMappings.erase(toFE);
270267
}
271268
}

‎clang/lib/Basic/Module.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -265,10 +265,10 @@ bool Module::fullModuleNameIs(ArrayRef<StringRef> nameParts) const {
265265
}
266266

267267
OptionalDirectoryEntryRef Module::getEffectiveUmbrellaDir() const {
268-
if (Umbrella && Umbrella.is<FileEntryRef>())
269-
return Umbrella.get<FileEntryRef>().getDir();
270-
if (Umbrella && Umbrella.is<DirectoryEntryRef>())
271-
return Umbrella.get<DirectoryEntryRef>();
268+
if (const auto *Hdr = std::get_if<FileEntryRef>(&Umbrella))
269+
return Hdr->getDir();
270+
if (const auto *Dir = std::get_if<DirectoryEntryRef>(&Umbrella))
271+
return *Dir;
272272
return std::nullopt;
273273
}
274274

‎clang/lib/Lex/HeaderSearch.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1623,7 +1623,7 @@ bool HeaderSearch::findUsableModuleForHeader(
16231623
ModuleMap::KnownHeader *SuggestedModule, bool IsSystemHeaderDir) {
16241624
if (needModuleLookup(RequestingModule, SuggestedModule)) {
16251625
// If there is a module that corresponds to this header, suggest it.
1626-
hasModuleMap(File.getName(), Root, IsSystemHeaderDir);
1626+
hasModuleMap(File.getNameAsRequested(), Root, IsSystemHeaderDir);
16271627
return suggestModule(*this, File, RequestingModule, SuggestedModule);
16281628
}
16291629
return true;

‎clang/lib/Lex/ModuleMap.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2426,7 +2426,8 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken,
24262426
Header.Kind = Map.headerRoleToKind(Role);
24272427

24282428
// Check whether we already have an umbrella.
2429-
if (Header.IsUmbrella && ActiveModule->Umbrella) {
2429+
if (Header.IsUmbrella &&
2430+
!std::holds_alternative<std::monostate>(ActiveModule->Umbrella)) {
24302431
Diags.Report(Header.FileNameLoc, diag::err_mmap_umbrella_clash)
24312432
<< ActiveModule->getFullModuleName();
24322433
HadError = true;
@@ -2523,7 +2524,7 @@ void ModuleMapParser::parseUmbrellaDirDecl(SourceLocation UmbrellaLoc) {
25232524
SourceLocation DirNameLoc = consumeToken();
25242525

25252526
// Check whether we already have an umbrella.
2526-
if (ActiveModule->Umbrella) {
2527+
if (!std::holds_alternative<std::monostate>(ActiveModule->Umbrella)) {
25272528
Diags.Report(DirNameLoc, diag::err_mmap_umbrella_clash)
25282529
<< ActiveModule->getFullModuleName();
25292530
HadError = true;

‎clang/lib/Serialization/ASTReader.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4175,7 +4175,7 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
41754175
return OutOfDate;
41764176
}
41774177

4178-
llvm::SmallPtrSet<FileEntryRef, 1> AdditionalStoredMaps;
4178+
ModuleMap::AdditionalModMapsSet AdditionalStoredMaps;
41794179
for (unsigned I = 0, N = Record[Idx++]; I < N; ++I) {
41804180
// FIXME: we should use input files rather than storing names.
41814181
std::string Filename = ReadPath(F, Record, Idx);

‎clang/test/Modules/Inputs/all-product-headers.yaml

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
{
2+
'version': 0,
3+
'case-sensitive': 'false',
4+
'roots': [
5+
{
6+
'type': 'directory',
7+
'name': "DUMMY_DIR/build/A.framework/PrivateHeaders"
8+
'contents': [
9+
{
10+
'type': 'file',
11+
'name': "A.h",
12+
'external-contents': "DUMMY_DIR/sources/A.h"
13+
}
14+
]
15+
},
16+
{
17+
'type': 'directory',
18+
'name': "DUMMY_DIR/build/A.framework/Modules"
19+
'contents': [
20+
{
21+
'type': 'file',
22+
'name': "module.modulemap",
23+
'external-contents': "DUMMY_DIR/build/module.modulemap"
24+
},
25+
{
26+
'type': 'file',
27+
'name': "module.private.modulemap",
28+
'external-contents': "DUMMY_DIR/build/module.private.modulemap"
29+
}
30+
]
31+
}
32+
]
33+
}

‎clang/test/Modules/modulemap-collision.m

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// REQUIRES: shell
2+
3+
// RUN: rm -rf %t
4+
// RUN: mkdir -p %t/sources %t/build
5+
// RUN: echo "// A.h" > %t/sources/A.h
6+
// RUN: echo "framework module A {}" > %t/sources/module.modulemap
7+
// RUN: echo "framework module A.Private { umbrella header \"A.h\" }" > %t/sources/module.private.modulemap
8+
// RUN: cp %t/sources/module.modulemap %t/build/module.modulemap
9+
// RUN: cp %t/sources/module.private.modulemap %t/build/module.private.modulemap
10+
11+
// RUN: sed -e "s:DUMMY_DIR:%t:g" %S/Inputs/all-product-headers.yaml > %t/build/all-product-headers.yaml
12+
// 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
13+
14+
// expected-no-diagnostics
15+
#import <A/A.h>

0 commit comments

Comments
 (0)