benlangmuir updated this revision to Diff 450313.
benlangmuir added a comment.
Herald added a subscriber: ormris.
Attempt to fix test failure seen on Windows. It revealed two bugs
- Avoid reusing the FileManager at the top-level in clang-scan-deps to make the
test behave as expected
- Make the FileManager return the cached redirecting entry, to match how it
behaves when it's a new file, and test this case explicitly.
I think it was mostly luck of exactly which file lookups happen that we didn't
hit this on other platforms.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131076/new/
https://reviews.llvm.org/D131076
Files:
clang/include/clang/Driver/Options.td
clang/include/clang/Frontend/FrontendOptions.h
clang/lib/Basic/FileManager.cpp
clang/lib/Frontend/CompilerInstance.cpp
clang/test/ClangScanDeps/modulemap-via-vfs.m
clang/test/Modules/submodule-in-private-mmap-vfs.m
clang/test/VFS/module-import.m
clang/unittests/Basic/FileManagerTest.cpp
Index: clang/unittests/Basic/FileManagerTest.cpp
===================================================================
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -340,6 +340,7 @@
auto F1Again = manager.getFileRef("dir/f1.cpp");
auto F1Also = manager.getFileRef("dir/f1-also.cpp");
auto F1Redirect = manager.getFileRef("dir/f1-redirect.cpp");
+ auto F1RedirectAgain = manager.getFileRef("dir/f1-redirect.cpp");
auto F2 = manager.getFileRef("dir/f2.cpp");
// Check Expected<FileEntryRef> for error.
@@ -347,6 +348,7 @@
ASSERT_FALSE(!F1Also);
ASSERT_FALSE(!F1Again);
ASSERT_FALSE(!F1Redirect);
+ ASSERT_FALSE(!F1RedirectAgain);
ASSERT_FALSE(!F2);
// Check names.
@@ -354,6 +356,7 @@
EXPECT_EQ("dir/f1.cpp", F1Again->getName());
EXPECT_EQ("dir/f1-also.cpp", F1Also->getName());
EXPECT_EQ("dir/f1.cpp", F1Redirect->getName());
+ EXPECT_EQ("dir/f1.cpp", F1RedirectAgain->getName());
EXPECT_EQ("dir/f2.cpp", F2->getName());
EXPECT_EQ("dir/f1.cpp", F1->getNameAsRequested());
@@ -363,6 +366,7 @@
EXPECT_EQ(&F1->getFileEntry(), *F1);
EXPECT_EQ(*F1, &F1->getFileEntry());
EXPECT_EQ(&F1->getFileEntry(), &F1Redirect->getFileEntry());
+ EXPECT_EQ(&F1->getFileEntry(), &F1RedirectAgain->getFileEntry());
EXPECT_NE(&F2->getFileEntry(), *F1);
EXPECT_NE(*F1, &F2->getFileEntry());
@@ -371,6 +375,7 @@
EXPECT_EQ(*F1, *F1Again);
EXPECT_EQ(*F1, *F1Redirect);
EXPECT_EQ(*F1Also, *F1Redirect);
+ EXPECT_EQ(*F1, *F1RedirectAgain);
EXPECT_NE(*F2, *F1);
EXPECT_NE(*F2, *F1Also);
EXPECT_NE(*F2, *F1Again);
@@ -381,6 +386,7 @@
EXPECT_FALSE(F1->isSameRef(*F1Redirect));
EXPECT_FALSE(F1->isSameRef(*F1Also));
EXPECT_FALSE(F1->isSameRef(*F2));
+ EXPECT_TRUE(F1Redirect->isSameRef(*F1RedirectAgain));
}
// getFile() Should return the same entry as getVirtualFile if the file actually
Index: clang/test/VFS/module-import.m
===================================================================
--- clang/test/VFS/module-import.m
+++ clang/test/VFS/module-import.m
@@ -1,6 +1,7 @@
-// RUN: rm -rf %t
+// RUN: rm -rf %t %t-unshared
// RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay.yaml > %t.yaml
// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s
@import not_real;
@@ -18,9 +19,12 @@
// Override the module map (vfsoverlay2 on top)
// RUN: sed -e "s@INPUT_DIR@%{/S:regex_replacement}/Inputs@g" -e "s@OUT_DIR@%{/t:regex_replacement}@g" %S/Inputs/vfsoverlay2.yaml > %t2.yaml
// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
+// RUN: %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -I %t -fsyntax-only %s
// vfsoverlay2 not present
// RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
// vfsoverlay2 on the bottom
// RUN: not %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
+// RUN: not %clang_cc1 -Werror -fmodules -fno-modules-share-filemanager -fimplicit-module-maps -fmodules-cache-path=%t-unshared -ivfsoverlay %t2.yaml -ivfsoverlay %t.yaml -I %t -fsyntax-only %s -DIMPORT2 2>&1 | FileCheck -check-prefix=CHECK-VFS2 %s
Index: clang/test/Modules/submodule-in-private-mmap-vfs.m
===================================================================
--- /dev/null
+++ clang/test/Modules/submodule-in-private-mmap-vfs.m
@@ -0,0 +1,38 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/vfs.json.in > %t/vfs.json
+// RUN: %clang_cc1 -fmodules -fno-modules-share-filemanager -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t -I%t/Virtual -ivfsoverlay %t/vfs.json -fsyntax-only %t/tu.m -verify
+
+//--- Dir1/module.modulemap
+
+//--- Dir2/module.private.modulemap
+module Foo_Private {}
+
+//--- vfs.json.in
+{
+ 'version': 0,
+ 'use-external-names': true,
+ 'roots': [
+ {
+ 'name': 'DIR/Virtual',
+ 'type': 'directory',
+ 'contents': [
+ {
+ 'name': 'module.modulemap',
+ 'type': 'file',
+ 'external-contents': 'DIR/Dir1/module.modulemap'
+ },
+ {
+ 'name': 'module.private.modulemap',
+ 'type': 'file',
+ 'external-contents': 'DIR/Dir2/module.private.modulemap'
+ }
+ ]
+ }
+ ]
+}
+
+//--- tu.m
+@import Foo_Private;
+// expected-no-diagnostics
Index: clang/test/ClangScanDeps/modulemap-via-vfs.m
===================================================================
--- clang/test/ClangScanDeps/modulemap-via-vfs.m
+++ clang/test/ClangScanDeps/modulemap-via-vfs.m
@@ -2,13 +2,15 @@
// RUN: split-file %s %t.dir
// RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/compile-commands.json.in > %t.dir/build/compile-commands.json
// RUN: sed -e "s|DIR|%/t.dir|g" %t.dir/build/vfs.yaml.in > %t.dir/build/vfs.yaml
-// RUN: clang-scan-deps -compilation-database %t.dir/build/compile-commands.json -j 1 -format experimental-full \
+// RUN: clang-scan-deps -compilation-database %t.dir/build/compile-commands.json \
+// RUN: -reuse-filemanager=0 -j 1 -format experimental-full \
// RUN: -mode preprocess-dependency-directives -generate-modules-path-args > %t.db
// RUN: %deps-to-rsp %t.db --module-name=A > %t.A.cc1.rsp
// RUN: cat %t.A.cc1.rsp | sed 's:\\\\\?:/:g' | FileCheck %s
// CHECK-NOT: build/module.modulemap
// CHECK: A/module.modulemap
+// CHECK-NOT: build/module.modulemap
//--- build/compile-commands.json.in
@@ -17,6 +19,11 @@
"directory": "DIR",
"command": "clang DIR/main.m -Imodules/A -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -ivfsoverlay build/vfs.yaml",
"file": "DIR/main.m"
+},
+{
+ "directory": "DIR",
+ "command": "clang DIR/main.m -Imodules/A -fmodules -Xclang -fno-modules-share-filemanager -fmodules-cache-path=DIR/module-cache-unshared -fimplicit-modules -fimplicit-module-maps -ivfsoverlay build/vfs.yaml",
+ "file": "DIR/main.m"
}
]
@@ -35,6 +42,7 @@
{
"version": 0,
"case-sensitive": "false",
+ "use-external-names": true,
"roots": [
{
"contents": [
Index: clang/lib/Frontend/CompilerInstance.cpp
===================================================================
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1213,11 +1213,16 @@
ImportingInstance.getDiagnosticClient()),
/*ShouldOwnClient=*/true);
- // Note that this module is part of the module build stack, so that we
- // can detect cycles in the module graph.
- Instance.setFileManager(&ImportingInstance.getFileManager());
+ if (FrontendOpts.ModulesShareFileManager) {
+ Instance.setFileManager(&ImportingInstance.getFileManager());
+ } else {
+ Instance.createFileManager(&ImportingInstance.getVirtualFileSystem());
+ }
Instance.createSourceManager(Instance.getFileManager());
SourceManager &SourceMgr = Instance.getSourceManager();
+
+ // Note that this module is part of the module build stack, so that we
+ // can detect cycles in the module graph.
SourceMgr.setModuleBuildStack(
ImportingInstance.getSourceManager().getModuleBuildStack());
SourceMgr.pushModuleBuildStack(ModuleName,
@@ -1303,12 +1308,16 @@
ModuleMapFile, ImportingInstance.getFileManager()))
ModuleMapFile = PublicMMFile;
+ // FIXME: Update header search to keep FileEntryRef rather than rely on
+ // getLastRef().
+ StringRef ModuleMapFilePath =
+ ModuleMapFile->getLastRef().getNameAsRequested();
+
// Use the module map where this module resides.
Result = compileModuleImpl(
ImportingInstance, ImportLoc, Module->getTopLevelModuleName(),
- FrontendInputFile(ModuleMapFile->getName(), IK, +Module->IsSystem),
- ModMap.getModuleMapFileForUniquing(Module)->getName(),
- ModuleFileName);
+ FrontendInputFile(ModuleMapFilePath, IK, +Module->IsSystem),
+ ModMap.getModuleMapFileForUniquing(Module)->getName(), ModuleFileName);
} else {
// FIXME: We only need to fake up an input file here as a way of
// transporting the module's directory to the module map parser. We should
Index: clang/lib/Basic/FileManager.cpp
===================================================================
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -212,13 +212,7 @@
if (!SeenFileInsertResult.first->second)
return llvm::errorCodeToError(
SeenFileInsertResult.first->second.getError());
- // Construct and return and FileEntryRef, unless it's a redirect to another
- // filename.
- FileEntryRef::MapValue Value = *SeenFileInsertResult.first->second;
- if (LLVM_LIKELY(Value.V.is<FileEntry *>()))
- return FileEntryRef(*SeenFileInsertResult.first);
- return FileEntryRef(*reinterpret_cast<const FileEntryRef::MapEntry *>(
- Value.V.get<const void *>()));
+ return FileEntryRef(*SeenFileInsertResult.first);
}
// We've not seen this before. Fill it in.
Index: clang/include/clang/Frontend/FrontendOptions.h
===================================================================
--- clang/include/clang/Frontend/FrontendOptions.h
+++ clang/include/clang/Frontend/FrontendOptions.h
@@ -356,6 +356,9 @@
/// Output (and read) PCM files regardless of compiler errors.
unsigned AllowPCMWithCompilerErrors : 1;
+ /// Whether to share the FileManager when building modules.
+ unsigned ModulesShareFileManager : 1;
+
CodeCompleteOptions CodeCompleteOpts;
/// Specifies the output format of the AST.
@@ -523,7 +526,7 @@
BuildingImplicitModuleUsesLock(true), ModulesEmbedAllFiles(false),
IncludeTimestamps(true), UseTemporary(true),
OutputPathIndependentPCM(false), AllowPCMWithCompilerErrors(false),
- TimeTraceGranularity(500) {}
+ ModulesShareFileManager(true), TimeTraceGranularity(500) {}
/// getInputKindForExtension - Return the appropriate input kind for a file
/// extension. For example, "c" would return Language::C.
Index: clang/include/clang/Driver/Options.td
===================================================================
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -6028,6 +6028,9 @@
Flag<["-"], "fallow-pch-with-different-modules-cache-path">,
HelpText<"Accept a PCH file that was created with a different modules cache path">,
MarshallingInfoFlag<PreprocessorOpts<"AllowPCHWithDifferentModulesCachePath">>;
+def fno_modules_share_filemanager : Flag<["-"], "fno-modules-share-filemanager">,
+ HelpText<"Disable sharing the FileManager when building a module implicitly">,
+ MarshallingInfoNegativeFlag<FrontendOpts<"ModulesShareFileManager">>;
def dump_deserialized_pch_decls : Flag<["-"], "dump-deserialized-decls">,
HelpText<"Dump declarations that are deserialized from PCH, for testing">,
MarshallingInfoFlag<PreprocessorOpts<"DumpDeserializedPCHDecls">>;
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
