-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang][deps] Load module map file from PCM #66389
Closed
+86
−20
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
[clang][deps] Load module map file from PCM
commit 6712f9799f6865aeb40163805cca5f9349cabc45
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -482,16 +482,9 @@ ModuleDepCollectorPP::handleTopLevelModule(const Module *M) { | |
MD.ID.ModuleName = M->getFullModuleName(); | ||
MD.IsSystem = M->IsSystem; | ||
|
||
ModuleMap &ModMapInfo = | ||
MDC.ScanInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap(); | ||
|
||
OptionalFileEntryRef ModuleMap = ModMapInfo.getModuleMapFileForUniquing(M); | ||
|
||
if (ModuleMap) { | ||
SmallString<128> Path = ModuleMap->getNameAsRequested(); | ||
ModMapInfo.canonicalizeModuleMapPath(Path); | ||
MD.ClangModuleMapFile = std::string(Path); | ||
} | ||
assert(!M->PresumedModuleMapFile.empty() && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a comment in Module.h: |
||
"Precompiled module missing presumed module map"); | ||
MD.ClangModuleMapFile = M->PresumedModuleMapFile; | ||
|
||
serialization::ModuleFile *MF = | ||
MDC.ScanInstance.getASTReader()->getModuleManager().lookup( | ||
|
@@ -505,7 +498,10 @@ ModuleDepCollectorPP::handleTopLevelModule(const Module *M) { | |
// handle it like normal. With explicitly built modules we don't need | ||
// to play VFS tricks, so replace it with the correct module map. | ||
if (StringRef(IFI.Filename).endswith("__inferred_module.map")) { | ||
MDC.addFileDep(MD, ModuleMap->getName()); | ||
auto FE = MDC.ScanInstance.getFileManager().getOptionalFileRef( | ||
MD.ClangModuleMapFile); | ||
assert(FE && "Module map of precompiled module still exists"); | ||
MDC.addFileDep(MD, FE->getName()); | ||
return; | ||
} | ||
MDC.addFileDep(MD, IFI.Filename); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
// This test checks that we report the module map that allowed inferring using | ||
// its on-disk path in file dependencies. | ||
|
||
// RUN: rm -rf %t | ||
// RUN: split-file %s %t | ||
|
||
//--- frameworks/Inferred.framework/Headers/Inferred.h | ||
//--- frameworks/Inferred.framework/Frameworks/Sub.framework/Headers/Sub.h | ||
//--- real/module.modulemap | ||
framework module * {} | ||
//--- vfsoverlay.json.template | ||
{ | ||
"version": 0, | ||
"case-sensitive": "false", | ||
"use-external-names": true, | ||
"roots": [ | ||
{ | ||
"contents": [ | ||
{ | ||
"external-contents": "DIR/real/module.modulemap", | ||
"name": "module.modulemap", | ||
"type": "file" | ||
} | ||
], | ||
"name": "DIR/frameworks", | ||
"type": "directory" | ||
} | ||
] | ||
} | ||
//--- tu.m | ||
#include <Inferred/Inferred.h> | ||
|
||
//--- cdb.json.template | ||
[{ | ||
"directory": "DIR", | ||
"file": "DIR/tu.m", | ||
"command": "clang -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache -F DIR/frameworks -ivfsoverlay DIR/vfsoverlay.json -c DIR/tu.m -o DIR/tu.o" | ||
}] | ||
|
||
// RUN: sed "s|DIR|%/t|g" %t/vfsoverlay.json.template > %t/vfsoverlay.json | ||
// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json | ||
// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json | ||
// RUN: cat %t/result.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t | ||
|
||
// CHECK: { | ||
// CHECK-NEXT: "modules": [ | ||
// CHECK-NEXT: { | ||
// CHECK-NEXT: "clang-module-deps": [], | ||
// CHECK-NEXT: "clang-modulemap-file": "[[PREFIX]]/frameworks/module.modulemap", | ||
// CHECK-NEXT: "command-line": [ | ||
// CHECK: ], | ||
// CHECK-NEXT: "context-hash": "{{.*}}", | ||
// CHECK-NEXT: "file-deps": [ | ||
// CHECK-NEXT: "[[PREFIX]]/frameworks/Inferred.framework/Frameworks/Sub.framework/Headers/Sub.h", | ||
// CHECK-NEXT: "[[PREFIX]]/frameworks/Inferred.framework/Headers/Inferred.h", | ||
// CHECK-NEXT: "[[PREFIX]]/real/module.modulemap" | ||
// CHECK-NEXT: ], | ||
// CHECK-NEXT: "name": "Inferred" | ||
// CHECK-NEXT: } | ||
// CHECK-NEXT: ], | ||
// CHECK-NEXT: "translation-units": [ | ||
// CHECK: ] | ||
// CHECK: } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure about this change: I seem to recall some folks are intentionally using relative paths in their invocations in order to be independent of a base path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you concerned because this would make the second hash in implicitly-built PCM path depend on CWD? The other place this now gets used is in the
ASTWriter
, but there it gets passed through itsAddPath()
, which makes it relative if needed/requested.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also impact explicit module invocations. I didn't realize the impact on path hash; good point that also seems like a concern. I didn't consider ASTWriter would make it relative, so at least that case wouldn't be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how would this impact explicit module invocations? These can still use relative module map paths, it's just that the compiler will use the absolute canonical path internally (IIUC in an non-observable way).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea that because the paths used by the scanner is now indirected through the (implicit) pcm that whatever ASTWriter does to make the path relative will apply to the explicit module invocation? Before your current changes the scanner was using
canonicalizeModuleMapPath
directly on the path in the invocation, which is what I was naively thinking of.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because
ASTReader::ReadPath()
does the inverse ofASTWriter::AddPath()
.I think we were both talking about different situations. You mean the explicit module invocations generated by the scanner? The canonical module map path would now be absolute. Previously, we'd generate relative paths for module maps found through a relative search paths, or besides includers that themselves were found through a relative path.
I'd be curious about the use-case for this, can you elaborate? Is it being able to reuse the same invocation in directories with different paths, potentially on different machines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this change is what I'm concerned about.
My understanding is that this can be used to make the .pcms and command-lines to build them portable across machines, e.g. for caching builds in tools like bazel. I don't know how much overlap there is between this use case and using the dependency scanner.
Maybe we can flip it around: where exactly do we need an absolute path with your changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I take that back. (It's been a while since I wrote this patch.) This function (and therefore the scanner) used to return absolute paths even before this patch because it always uses
FileManager::getCanonicalName()
.So the only thing this patch changes is what gets serialized into the PCM file.
When I started canonicalizing the module map path before storing it into the PCM, some tests started unexpectedly failing with
ASTWriter
reporting inconsistencies.When I looked into it, I found it was due to the fact that
FileManager::getCanonicalName()
just forwards the path it's given tollvm::vfs::FileSystem::getRealPath()
. The problem is, the VFS might have a different idea as to what the CWD is, which breaks with relative module map paths and-working-directory
(used intests/Modules/relative-import-path.c
).Another issue that popped up was that this function calls
replace_path_prefix(Path, Dir, CanonicalDir)
. IfDir
is empty, it just slaps the absolute real path for CWD in front of the filename without any separator.Making the path absolute early on with
FileManager
's CWD solved both of those issues. (No usage of VFS's CWD, no emptyDir
.)So I think given all of that, my concerns are:
FileManager::makeAbsolutePath()
inModuleMap::canonicalizeModuleMapPath()
or inFileManager::getCanonicalName()
?ASTWriter::AddPath()
with the real path problematic? It only performs textual prefix match with a directory path that's not necessarily the real path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, my bad. As the person who wrote that code I should've realized this was already absolute.
I think the correct behaviour would be that
FileManager::getCanonicalName
would useFixupRelativePath
to apply-working-directory
, but not usemakeAbsolutePath
since that would be inconsistent with how it handles paths in other APIs. The VFS itself is responsible for resolving CWD ingetRealPath
.That being said, if this causes other issues I'm fine with your current approach in the meantime.
I don't know how we avoid that without the same issues that motivated canonicalizing in the first place. We don't want to be compiling separate modules for /a/Foo, /a/foo, /a/b/../foo, /symlink/foo, etc.