Address review feedback

* Fix logic error when deserializing input file
* Also add a check for absolute paths in `isPathInStableDir`
* Always passdown filenameAsRequested & externalfilename

Author: cyndyishida

clang/include/clang/Serialization/ASTReader.h

@@ -242,9 +242,9 @@ class ASTReaderListener {
   /// paths, for example when deserializing input files from AST files.
   ///
   /// \returns true to continue receiving the next input file, false to stop.
-  virtual bool visitInputFile(StringRef FilenameAsRequested, StringRef Filename,
-                              bool isSystem, bool isOverridden,
-                              bool isExplicitModule) {
+  virtual bool visitInputFile(StringRef FilenameAsRequested,
+                              StringRef ExternalFilename, bool isSystem,
+                              bool isOverridden, bool isExplicitModule) {
     return true;
   }
 

clang/lib/Frontend/FrontendActions.cpp

@@ -777,9 +777,9 @@ namespace {
     /// Indicates that the AST file contains particular input file.
     ///
     /// \returns true to continue receiving the next input file, false to stop.
-    bool visitInputFile(StringRef FilenameAsRequested, StringRef Filename,
-                        bool isSystem, bool isOverridden,
-                        bool isExplicitModule) override {
+    bool visitInputFile(StringRef FilenameAsRequested,
+                        StringRef ExternalFilename, bool isSystem,
+                        bool isOverridden, bool isExplicitModule) override {
 
       Out.indent(2) << "Input file: " << FilenameAsRequested;
 

clang/lib/Serialization/ASTReader.cpp

@@ -2616,6 +2616,18 @@ bool ASTReader::shouldDisableValidationForFile(
   return false;
 }
 
+namespace {
+
+std::pair<StringRef, StringRef>
+getUnresolvedInputFilenames(const ASTReader::RecordData &Record,
+                            const StringRef InputBlob) {
+  uint16_t AsRequestedLength = Record[7];
+  return {InputBlob.substr(0, AsRequestedLength),
+          InputBlob.substr(AsRequestedLength)};
+}
+
+} // namespace
+
 InputFileInfo ASTReader::getInputFileInfo(ModuleFile &F, unsigned ID) {
   // If this ID is bogus, just return an empty input file.
   if (ID == 0 || ID > F.InputFileInfosLoaded.size())
@@ -2659,11 +2671,12 @@ InputFileInfo ASTReader::getInputFileInfo(ModuleFile &F, unsigned ID) {
   R.Transient = static_cast<bool>(Record[4]);
   R.TopLevel = static_cast<bool>(Record[5]);
   R.ModuleMap = static_cast<bool>(Record[6]);
-  uint16_t AsRequestedLength = Record[7];
-  R.UnresolvedImportedFilenameAsRequested = Blob.substr(0, AsRequestedLength);
-  R.UnresolvedImportedFilename = Blob.substr(AsRequestedLength);
-  if (R.UnresolvedImportedFilename.empty())
-    R.UnresolvedImportedFilename = R.UnresolvedImportedFilenameAsRequested;
+  auto [UnresolvedFilenameAsRequested, UnresolvedFilename] =
+      getUnresolvedInputFilenames(Record, Blob);
+  R.UnresolvedImportedFilenameAsRequested = UnresolvedFilenameAsRequested;
+  R.UnresolvedImportedFilename = UnresolvedFilename.empty()
+                                     ? UnresolvedFilenameAsRequested
+                                     : UnresolvedFilename;
 
   Expected<llvm::BitstreamEntry> MaybeEntry = Cursor.advance();
   if (!MaybeEntry) // FIXME this drops errors on the floor.
@@ -5704,6 +5717,11 @@ bool ASTReader::readASTFileControlBlock(
   bool DoneWithControlBlock = false;
   SmallString<0> PathBuf;
   PathBuf.reserve(256);
+  // Additional path buffer to use when multiple paths need to be resolved.
+  // For example, when deserializing input files that contains a path that was
+  // resolved from a vfs overlay and an external location.
+  SmallString<0> AdditionalPathBuf;
+  AdditionalPathBuf.reserve(256);
   while (!DoneWithControlBlock) {
     Expected<llvm::BitstreamEntry> MaybeEntry = Stream.advance();
     if (!MaybeEntry) {
@@ -5835,12 +5853,20 @@ bool ASTReader::readASTFileControlBlock(
           break;
         case INPUT_FILE:
           bool Overridden = static_cast<bool>(Record[3]);
-          size_t FilenameLen = ModuleDir.size() + Record[7] + 1;
-          auto Filename = ResolveImportedPath(PathBuf, Blob, ModuleDir);
-          StringRef FilenameAsRequested = Filename->substr(0, FilenameLen);
-          StringRef ExternalFilename = Filename->substr(FilenameLen);
+          auto [UnresolvedFilenameAsRequested, UnresolvedFilename] =
+              getUnresolvedInputFilenames(Record, Blob);
+          auto FilenameAsRequestedBuf = ResolveImportedPath(
+              PathBuf, UnresolvedFilenameAsRequested, ModuleDir);
+          StringRef Filename;
+          if (UnresolvedFilename.empty())
+            Filename = *FilenameAsRequestedBuf;
+          else {
+            auto FilenameBuf = ResolveImportedPath(
+                AdditionalPathBuf, UnresolvedFilename, ModuleDir);
+            Filename = *FilenameBuf;
+          }
           shouldContinue = Listener.visitInputFile(
-              FilenameAsRequested, ExternalFilename, isSystemFile, Overridden,
+              *FilenameAsRequestedBuf, Filename, isSystemFile, Overridden,
               /*IsExplicitModule=*/false);
           break;
         }

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

@@ -136,11 +136,8 @@ class PrebuiltModuleListener : public ASTReaderListener {
         !PrebuiltModulesASTMap[CurrentFile].isInStableDir())
       return false;
 
-    const StringRef FileToUse =
-        ExternalFilename.empty() ? FilenameAsRequested : ExternalFilename;
-
     PrebuiltModulesASTMap[CurrentFile].setInStableDir(
-        isPathInStableDir(StableDirs, FileToUse));
+        isPathInStableDir(StableDirs, ExternalFilename));
     return PrebuiltModulesASTMap[CurrentFile].isInStableDir();
   }
 

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

@@ -228,11 +228,14 @@ void dependencies::resetBenignCodeGenOptions(frontend::ActionKind ProgramAction,
 
 bool dependencies::isPathInStableDir(const ArrayRef<StringRef> Directories,
                                      const StringRef Input) {
+  using namespace llvm::sys;
+
+  if (!path::is_absolute(Input))
+    return false;
+
   auto PathStartsWith = [](StringRef Prefix, StringRef Path) {
-    auto PrefixIt = llvm::sys::path::begin(Prefix),
-         PrefixEnd = llvm::sys::path::end(Prefix);
-    for (auto PathIt = llvm::sys::path::begin(Path),
-              PathEnd = llvm::sys::path::end(Path);
+    auto PrefixIt = path::begin(Prefix), PrefixEnd = path::end(Prefix);
+    for (auto PathIt = path::begin(Path), PathEnd = path::end(Path);
          PrefixIt != PrefixEnd && PathIt != PathEnd; ++PrefixIt, ++PathIt) {
       if (*PrefixIt != *PathIt)
         return false;

clang/test/ClangScanDeps/prebuilt-modules-in-stable-dirs.c

@@ -2,7 +2,7 @@
 ///   resolve `is-in-stable-directories` correctly. 
 /// The steps are: 
 /// 1. Scan dependencies to build the PCH. One of the module's depend on header 
-///   that is seemingly from the sysroot. However, it depends on a local header that is overlaid.. 
+///   that is seemingly from the sysroot. However, it depends on a local header that is overlaid.
 /// 2. Build the PCH & dependency PCMs.
 /// 3. Scan a source file that transitively depends on the same modules as the pcm.