[clang] Delay normalization of -fmodules-cache-path

Author: jansvoboda11

clang/include/clang/Driver/Options.td

@@ -3281,7 +3281,8 @@ defm declspec : BoolOption<"f", "declspec",
 def fmodules_cache_path : Joined<["-"], "fmodules-cache-path=">, Group<i_Group>,
   Flags<[]>, Visibility<[ClangOption, CC1Option]>,
   MetaVarName<"<directory>">,
-  HelpText<"Specify the module cache path">;
+  HelpText<"Specify the module cache path">,
+  MarshallingInfoString<HeaderSearchOpts<"ModuleCachePath">>;
 def fmodules_user_build_path : Separate<["-"], "fmodules-user-build-path">, Group<i_Group>,
   Flags<[]>, Visibility<[ClangOption, CC1Option]>,
   MetaVarName<"<directory>">,

clang/lib/Frontend/CompilerInstance.cpp

@@ -548,9 +548,14 @@ void CompilerInstance::createPreprocessor(TranslationUnitKind TUKind) {
 }
 
 std::string CompilerInstance::getSpecificModuleCachePath(StringRef ModuleHash) {
+  if (getHeaderSearchOpts().ModuleCachePath.empty())
+    return "";
+
   // Set up the module path, including the hash for the module-creation options.
   SmallString<256> SpecificModuleCache(getHeaderSearchOpts().ModuleCachePath);
-  if (!SpecificModuleCache.empty() && !getHeaderSearchOpts().DisableModuleHash)
+  FileMgr->makeAbsolutePath(SpecificModuleCache);
+  llvm::sys::path::remove_dots(SpecificModuleCache);
+  if (!getHeaderSearchOpts().DisableModuleHash)
     llvm::sys::path::append(SpecificModuleCache, ModuleHash);
   return std::string(SpecificModuleCache);
 }

clang/lib/Frontend/CompilerInvocation.cpp

@@ -3315,9 +3315,6 @@ static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
   if (Opts.UseLibcxx)
     GenerateArg(Consumer, OPT_stdlib_EQ, "libc++");
 
-  if (!Opts.ModuleCachePath.empty())
-    GenerateArg(Consumer, OPT_fmodules_cache_path, Opts.ModuleCachePath);
-
   for (const auto &File : Opts.PrebuiltModuleFiles)
     GenerateArg(Consumer, OPT_fmodule_file, File.first + "=" + File.second);
 
@@ -3420,8 +3417,7 @@ static void GenerateHeaderSearchArgs(const HeaderSearchOptions &Opts,
 }
 
 static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
-                                  DiagnosticsEngine &Diags,
-                                  const std::string &WorkingDir) {
+                                  DiagnosticsEngine &Diags) {
   unsigned NumErrorsBefore = Diags.getNumErrors();
 
   HeaderSearchOptions *HeaderSearchOpts = &Opts;
@@ -3434,17 +3430,6 @@ static bool ParseHeaderSearchArgs(HeaderSearchOptions &Opts, ArgList &Args,
   if (const Arg *A = Args.getLastArg(OPT_stdlib_EQ))
     Opts.UseLibcxx = (strcmp(A->getValue(), "libc++") == 0);
 
-  // Canonicalize -fmodules-cache-path before storing it.
-  SmallString<128> P(Args.getLastArgValue(OPT_fmodules_cache_path));
-  if (!(P.empty() || llvm::sys::path::is_absolute(P))) {
-    if (WorkingDir.empty())
-      llvm::sys::fs::make_absolute(P);
-    else
-      llvm::sys::fs::make_absolute(WorkingDir, P);
-  }
-  llvm::sys::path::remove_dots(P);
-  Opts.ModuleCachePath = std::string(P);
-
   // Only the -fmodule-file=<name>=<file> form.
   for (const auto *A : Args.filtered(OPT_fmodule_file)) {
     StringRef Val = A->getValue();
@@ -5021,8 +5006,7 @@ bool CompilerInvocation::CreateFromArgsImpl(
   InputKind DashX = Res.getFrontendOpts().DashX;
   ParseTargetArgs(Res.getTargetOpts(), Args, Diags);
   llvm::Triple T(Res.getTargetOpts().Triple);
-  ParseHeaderSearchArgs(Res.getHeaderSearchOpts(), Args, Diags,
-                        Res.getFileSystemOpts().WorkingDir);
+  ParseHeaderSearchArgs(Res.getHeaderSearchOpts(), Args, Diags);
   if (Res.getFrontendOpts().GenReducedBMI ||
       Res.getFrontendOpts().ProgramAction ==
           frontend::GenerateReducedModuleInterface ||

clang/lib/Serialization/ASTWriter.cpp

@@ -1185,51 +1185,36 @@ static bool cleanPathForOutput(FileManager &FileMgr,
   return Changed | llvm::sys::path::remove_dots(Path);
 }
 
-/// Adjusts the given filename to only write out the portion of the
-/// filename that is not part of the system root directory.
+/// Adjusts the given filename to only write out the portion of the filename
+/// that is not part of the base directory.
 ///
 /// \param Filename the file name to adjust.
 ///
-/// \param BaseDir When non-NULL, the PCH file is a relocatable AST file and
-/// the returned filename will be adjusted by this root directory.
+/// \param BasePath When not empty, the AST file is relocatable and the returned
+/// filename will be adjusted to be relative to this path.
 ///
-/// \returns either the original filename (if it needs no adjustment) or the
-/// adjusted filename (which points into the @p Filename parameter).
-static const char *
-adjustFilenameForRelocatableAST(const char *Filename, StringRef BaseDir) {
-  assert(Filename && "No file name to adjust?");
-
-  if (BaseDir.empty())
-    return Filename;
-
-  // Verify that the filename and the system root have the same prefix.
-  unsigned Pos = 0;
-  for (; Filename[Pos] && Pos < BaseDir.size(); ++Pos)
-    if (Filename[Pos] != BaseDir[Pos])
-      return Filename; // Prefixes don't match.
-
-  // We hit the end of the filename before we hit the end of the system root.
-  if (!Filename[Pos])
-    return Filename;
-
-  // If there's not a path separator at the end of the base directory nor
-  // immediately after it, then this isn't within the base directory.
-  if (!llvm::sys::path::is_separator(Filename[Pos])) {
-    if (!llvm::sys::path::is_separator(BaseDir.back()))
-      return Filename;
-  } else {
-    // If the file name has a '/' at the current position, skip over the '/'.
-    // We distinguish relative paths from absolute paths by the
-    // absence of '/' at the beginning of relative paths.
-    //
-    // FIXME: This is wrong. We distinguish them by asking if the path is
-    // absolute, which isn't the same thing. And there might be multiple '/'s
-    // in a row. Use a better mechanism to indicate whether we have emitted an
-    // absolute or relative path.
-    ++Pos;
-  }
+/// \returns true when \c Filename was adjusted, false otherwise.
+static bool adjustFilenameForRelocatableAST(SmallVectorImpl<char> &Filename,
+                                            StringRef BasePath) {
+  auto FileIt = llvm::sys::path::begin({Filename.begin(), Filename.size()});
+  auto FileEnd = llvm::sys::path::end({Filename.begin(), Filename.size()});
+
+  auto BaseIt = llvm::sys::path::begin(BasePath);
+  auto BaseEnd = llvm::sys::path::end(BasePath);
+
+  for (; FileIt != FileEnd && BaseIt != BaseEnd; ++FileIt, ++BaseIt)
+    if (*FileIt != *BaseIt)
+      return false;
+
+  if (FileIt == FileEnd)
+    return false;
+
+  SmallString<128> Clean;
+  for (; FileIt != FileEnd; ++FileIt)
+    llvm::sys::path::append(Clean, *FileIt);
 
-  return Filename + Pos;
+  Filename.assign(Clean.begin(), Clean.end());
+  return true;
 }
 
 std::pair<ASTFileSignature, ASTFileSignature>
@@ -1712,7 +1697,7 @@ void ASTWriter::WriteControlBlock(Preprocessor &PP, StringRef isysroot) {
 
   AddString(HSOpts.Sysroot, Record);
   AddString(HSOpts.ResourceDir, Record);
-  AddString(HSOpts.ModuleCachePath, Record);
+  AddPath(HSOpts.ModuleCachePath, Record);
   AddString(HSOpts.ModuleUserBuildPath, Record);
   Record.push_back(HSOpts.DisableModuleHash);
   Record.push_back(HSOpts.ImplicitModuleMaps);
@@ -5342,16 +5327,8 @@ bool ASTWriter::PreparePathForOutput(SmallVectorImpl<char> &Path) {
     return false;
 
   bool Changed = cleanPathForOutput(PP->getFileManager(), Path);
-
-  // Remove a prefix to make the path relative, if relevant.
-  const char *PathBegin = Path.data();
-  const char *PathPtr =
-      adjustFilenameForRelocatableAST(PathBegin, BaseDirectory);
-  if (PathPtr != PathBegin) {
-    Path.erase(Path.begin(), Path.begin() + (PathPtr - PathBegin));
+  if (adjustFilenameForRelocatableAST(Path, BaseDirectory))
     Changed = true;
-  }
-
   return Changed;
 }
 

clang/test/Modules/modules-cache-path-canonicalization-output.c

@@ -0,0 +1,18 @@
+// This checks that implicitly-built modules produce identical PCM
+// files regardless of the spelling of the same module cache path.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fsyntax-only %t/tu.c \
+// RUN:   -fmodules-cache-path=%t/cache -fdisable-module-hash
+// RUN: mv %t/cache/M.pcm %t/M.pcm
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fsyntax-only %t/tu.c \
+// RUN:   -fmodules-cache-path=%t/./cache -fdisable-module-hash
+// RUN: diff %t/./cache/M.pcm %t/M.pcm
+
+//--- tu.c
+#include "M.h"
+//--- M.h
+//--- module.modulemap
+module M { header "M.h" }