[ASTImporter] Improve import of FileID.

[balazske]

Summary

Even if the content cache has a directory and filename, it may be a virtual file.
The old code returned with error in this case, but it is worth to try to handle
the file as it were a memory buffer.

[balazske]

This change is needed for D53818 to make LLDB tests pass.

[martong]

Ping

[martong]

Ping.

Please raise your objections if you have any until the 4th of March (that date we are going to commit if there are no objections). Also, please let us know if you find this deadline too strict.

[efriedma]

I don't know anything about this particular patch, but you aren't allowed to set deadlines like that; the patch review process requires that the patch is actually reviewed. If you aren't getting a response, ask on cfe-dev.

[martong]

In D57590#1411055, @efriedma wrote:

I don't know anything about this particular patch, but you aren't allowed to set deadlines like that; the patch review process requires that the patch is actually reviewed. If you aren't getting a response, ask on cfe-dev.

Okay sorry about that, I will not commit until we get an approve. I think I was mislead by the "LLVM Developer Policy" (https://llvm.org/docs/DeveloperPolicy.html)

Do not assume silent approval, or request active objections to the patch with a deadline.

I interpreted this as after several pings setting a deadline is okay, especially if the patch is small and getting old.
Perhaps the policy should be updated to avoid such misinterpretation in the future.

Unfortunately this part of Clang (ASTImporter) has quite a few active developers other than my colleges. Obviously my colleges could review these patches (and they have done that in our fork) but we thought it would be better to have accept from devs of other organizations. One of the clients of ASTImporter is cross translation unit (CTU) static analysis, the other is LLDB. We hope to have more users and developers of CTU by reaching a point where it is mature enough to attract more developers. At the moment CTU analysis is not successful even on a simple C project like tmux with the upstream master. But that is successful on complex C++ projects like protobuf since a long time in our fork. Upstreaming to master takes painfully too much time. Our fork is already ahead at least 25 commits and it is growing.

[shafik]

LGTM I don't see any LLDB test regressions but I would prefer another reviewer as well.

I am going to be modifying this soon to fix some issues w/ handling built-ins.

[a_sidorin]

Hi Gabor,
Sorry, missed the patch. It looks mostly good, but do we have any test for this?

[balazske]

Currently there is no direct test for ASTImporter FileID (and other non-Decl) import. These are tested indirectly by other import tests. There is a failing macOS LLDB test without this fix, after D53818 is applied.

lib/AST/ASTImporter.cpp

if (FromSLoc.isExpansion()) {
     const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
     if (Cache->OrigEntry && Cache->OrigEntry->getDir()) {
       // FIXME: We probably want to use getVirtualFile(), so we don't hit the
       // disk again
       // FIXME: We definitely want to re-use the existing MemoryBuffer, rather
       // than mmap the files several times.
       const FileEntry *Entry =
           ToFileManager.getFile(Cache->OrigEntry->getName());
-      if (!Entry)
-        return {};
+      // FIXME: The filename may be a virtual name that does probably not
+      // point to a valid file and we get no Entry here. In this case try with
+      // the memory buffer below.
+      if (Entry)
         ToID = ToSM.createFileID(Entry, ToIncludeLoc,
                                  FromSLoc.getFile().getFileCharacteristic());
-    } else {
+    }
+    if (ToID.isInvalid()) {
       // FIXME: We want to re-use the existing MemoryBuffer!
-      const llvm::MemoryBuffer *FromBuf =
-          Cache->getBuffer(FromContext.getDiagnostics(), FromSM);
+      bool Invalid = true;
+      const llvm::MemoryBuffer *FromBuf = Cache->getBuffer(
+          FromContext.getDiagnostics(), FromSM, SourceLocation{}, &Invalid);
+      if (!FromBuf || Invalid)
+        return {};
+
       std::unique_ptr<llvm::MemoryBuffer> ToBuf =
           llvm::MemoryBuffer::getMemBufferCopy(FromBuf->getBuffer(),
                                                FromBuf->getBufferIdentifier());
       ToID = ToSM.createFileID(std::move(ToBuf),
                                FromSLoc.getFile().getFileCharacteristic());
     }
   }
 
+  assert(ToID.isValid() && "Unexpected invalid fileID was created.");
+
   ImportedFileIDs[FromID] = ToID;
   return ToID;
 }
 
 Expected<CXXCtorInitializer *>
 ASTImporter::Import_New(CXXCtorInitializer *From) {
   CXXCtorInitializer *To = Import(From);
   if (!To && From)