[clang] [clang][deps] Load module map file from PCM (PR #66389)

Jan Svoboda via cfe-commits cfe-commits at lists.llvm.org
Fri Sep 15 12:06:38 PDT 2023


================
@@ -1307,6 +1307,9 @@ void ModuleMap::setInferredModuleAllowedBy(Module *M,
 
 std::error_code
 ModuleMap::canonicalizeModuleMapPath(SmallVectorImpl<char> &Path) {
+  FileManager &FM = SourceMgr.getFileManager();
+  FM.makeAbsolutePath(Path);
----------------
jansvoboda11 wrote:

> > 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.
> 
> Yes, this change is what I'm concerned about.

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](https://github.com/llvm/llvm-project/blob/c60ccfbb898148449946f82cbac6140f1e01cb12/clang/lib/Serialization/ASTReader.cpp#L4093).

When I looked into it, I found it was due to the fact that `FileManager::getCanonicalName()` just forwards the path it's given to `llvm::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 in `tests/Modules/relative-import-path.c`).

Another issue that popped up was that this function calls `replace_path_prefix(Path, Dir, CanonicalDir)`. If `Dir` 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 empty `Dir`.)

So I think given all of that, my concerns are:
* Should we call `FileManager::makeAbsolutePath()` in `ModuleMap::canonicalizeModuleMapPath()` or in `FileManager::getCanonicalName()`?
* Is using `ASTWriter::AddPath()` with the real path problematic? It only performs textual prefix match with a directory path that's not necessarily the real path.

https://github.com/llvm/llvm-project/pull/66389


More information about the cfe-commits mailing list