Fix FileManager invalidation issue for file redirect

[asuhan]

Summary

Insertion into SeenFileEntries can invalidate iterators, we need to do
another lookup on the re-intern path.

[dexonsmith]

clang/lib/Basic/FileManager.cpp

271–290

It's a little scary that NamedFileEnt is invalid after this, since there's a fair bit of code that follows. I could imagine this regressing. I suggest changing NamedFileEnt to a pointer that gets updated here.

[asuhan]

Make NamedFileEnt a pointer and update it.

[arphaman]

Thanks! LGTM.

[asuhan]

I don't have commit access, could you land this @arphaman? Thanks!

[asuhan]

Ping.

[compnerd]

GIT rb314414570c

clang/lib/Basic/FileManager.cpp

if (!SeenFileInsertResult.second) {
     FileEntry *FE;
     if (LLVM_LIKELY(FE = Value.dyn_cast<FileEntry *>()))
       return FileEntryRef(SeenFileInsertResult.first->first(), *FE);
     return getFileRef(*Value.get<const StringRef *>(), openFile, CacheFailure);
   }
 
   // We've not seen this before. Fill it in.
   ++NumFileCacheMisses;
-  auto &NamedFileEnt = *SeenFileInsertResult.first;
-  assert(!NamedFileEnt.second && "should be newly-created");
+  auto *NamedFileEnt = &*SeenFileInsertResult.first;
+  assert(!NamedFileEnt->second && "should be newly-created");
 
   // Get the null-terminated file name as stored as the key of the
   // SeenFileEntries map.
-  StringRef InterndFileName = NamedFileEnt.first();
+  StringRef InterndFileName = NamedFileEnt->first();
 
   // Look up the directory for the file.  When looking up something like
   // sys/foo.h we'll discover all of the search directories that have a 'sys'
   // subdirectory.  This will let us avoid having to waste time on known-to-fail
   // searches when we go to find sys/bar.h, because all the search directories
   // without a 'sys' subdir will get a cached failure result.
   auto DirInfoOrErr = getDirectoryFromFile(*this, Filename, CacheFailure);
   if (!DirInfoOrErr) { // Directory doesn't exist, file can't exist.
     if (CacheFailure)
-      NamedFileEnt.second = DirInfoOrErr.getError();
+      NamedFileEnt->second = DirInfoOrErr.getError();
     else
       SeenFileEntries.erase(Filename);
 
     return llvm::errorCodeToError(DirInfoOrErr.getError());
   }
   const DirectoryEntry *DirInfo = *DirInfoOrErr;
 
   // FIXME: Use the directory info to prune this, before doing the stat syscall.
   // FIXME: This will reduce the # syscalls.
 
   // Check to see if the file exists.
   std::unique_ptr<llvm::vfs::File> F;
   llvm::vfs::Status Status;
   auto statError = getStatValue(InterndFileName, Status, true,
                                 openFile ? &F : nullptr);
   if (statError) {
     // There's no real file at the given path.
     if (CacheFailure)
-      NamedFileEnt.second = statError;
+      NamedFileEnt->second = statError;
     else
       SeenFileEntries.erase(Filename);
 
     return llvm::errorCodeToError(statError);
   }
 
   assert((openFile || !F) && "undesired open file");
 
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
   FileEntry &UFE = UniqueRealFiles[Status.getUniqueID()];
 
-  NamedFileEnt.second = &UFE;
+  NamedFileEnt->second = &UFE;
 
   // If the name returned by getStatValue is different than Filename, re-intern
   // the name.
   if (Status.getName() != Filename) {
     auto &NewNamedFileEnt =
         *SeenFileEntries.insert({Status.getName(), &UFE}).first;
     assert((*NewNamedFileEnt.second).get<FileEntry *>() == &UFE &&
            "filename from getStatValue() refers to wrong file");
     InterndFileName = NewNamedFileEnt.first().data();
     // In addition to re-interning the name, construct a redirecting seen file
     // entry, that will point to the name the filesystem actually wants to use.
     StringRef *Redirect = new (CanonicalNameStorage) StringRef(InterndFileName);
-    NamedFileEnt.second = Redirect;
+    auto SeenFileInsertResultIt = SeenFileEntries.find(Filename);
+    assert(SeenFileInsertResultIt != SeenFileEntries.end() &&
+           "unexpected SeenFileEntries cache miss");
+    SeenFileInsertResultIt->second = Redirect;
+    NamedFileEnt = &*SeenFileInsertResultIt;
   }
 
   if (UFE.isValid()) { // Already have an entry with this inode, return it.
 
     // FIXME: this hack ensures that if we look up a file by a virtual path in
     // the VFS that the getDir() will have the virtual path, even if we found
     // the file by a 'real' path first. This is required in order to find a
     // module's structure when its headers/module map are mapped in the VFS.
     // We should remove this as soon as we can properly support a file having
     // multiple names.