Handle built-in when importing SourceLocation and FileID

[shafik]

Summary

Currently when we see a built-in we try and import the include location. Instead what we do now is find the buffer like we do for the invalid case and copy that over to the to context.

[teemperor]

I think the idea of the patch is right. Not sure tough if having Import take a second parameter is consistent with the other Import functions (and if that even matters).

include/clang/AST/ASTImporter.h

340

Import_New should probably also get the new paramater.

342

IsBuiltin, not isBuiltin

[lebedev.ri]

Is there a test missing here?

[martong]

Shafik, this looks good to me, once teemperor's comments are addressed.
Note, I added @balazske as a reviewer, he recently worked with importing the FileIDs, he may have some comments.

[balazske]

include/clang/AST/ASTImporter.h

342

IsBuiltin = false should be the correct formatting.

lib/AST/ASTImporter.cpp

8249–8250

The new parameter should be added here too.

8284

The Cache can be moved into this block and the block to else if.

8301

Is it possible that in the isBuiltin true case this part of the code does run (always) without assertion or other error and the result is always an invalid ToID? (If yes the whole change is not needed, so I want to know what was the reason for this change, was there any crash or bug.)

[shafik]

Addressed comments on formatting and missing changes to the Import_New version of the code.

[shafik]

In D58743#1413249, @lebedev.ri wrote:

Is there a test missing here?

This origin of this fix is the LLDB expression parsing issue. We were not able to reduce to a test we could put in ASTImpoterTest.cpp but we have a LLDB test that exercises this issue and will pass once this fix is in place.

See this differential https://reviews.llvm.org/D58790

lib/AST/ASTImporter.cpp

8284

I am not sure I understand what you are asking. Do you mean just duplicate the

const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();

in both blocks?

8301

This change was the result of a assert where evaluating an expression resulted in the built-in being imported and it would assert in SourceManager when when Import(SourceLocation....) attempted getDecomposedLoc at the next step.

AFAICT the The Preprocessor sets it up the buffer here and then creates the FileID for in the SourceManager and copying over the buffer should be the correct thing to do since that seems to be sufficient.

I also while testing verified that the BufferIndetifer() did indeed equal "<built-in>" similar to this check in SourceManager.

[balazske]

Looks good now.

[shafik]

@teemperor ping

[teemperor]

LGTM, thanks!

include/clang/AST/ASTImporter.h

public:
     // FIXME: Remove this version.
     Selector Import(Selector FromSel);
 
     /// Import the given file ID from the "from" context into the
     /// "to" context.
     ///
     /// \returns The equivalent file ID in the source manager of the "to"
     /// context, or the import error.
-    llvm::Expected<FileID> Import_New(FileID);
+    llvm::Expected<FileID> Import_New(FileID, bool IsBuiltin = false);
     // FIXME: Remove this version.
-    FileID Import(FileID);
+    FileID Import(FileID, bool IsBuiltin = false);
 
     /// Import the given C++ constructor initializer from the "from"
     /// context into the "to" context.
     ///
     /// \returns The equivalent initializer in the "to" context, or the import
     /// error.
     llvm::Expected<CXXCtorInitializer *>
     Import_New(CXXCtorInitializer *FromInit);

lib/AST/ASTImporter.cpp

if (ToLoc.isInvalid() && !FromLoc.isInvalid())
     return llvm::make_error<ImportError>();
   return ToLoc;
 }
 SourceLocation ASTImporter::Import(SourceLocation FromLoc) {
   if (FromLoc.isInvalid())
     return {};
 
   SourceManager &FromSM = FromContext.getSourceManager();
+  bool IsBuiltin = FromSM.isWrittenInBuiltinFile(FromLoc);
 
   std::pair<FileID, unsigned> Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  FileID ToFileID = Import(Decomposed.first);
+  FileID ToFileID = Import(Decomposed.first, IsBuiltin);
   if (ToFileID.isInvalid())
     return {};
   SourceManager &ToSM = ToContext.getSourceManager();
   return ToSM.getComposedLoc(ToFileID, Decomposed.second);
 }
 
 Expected<SourceRange> ASTImporter::Import_New(SourceRange FromRange) {
   SourceRange ToRange = Import(FromRange);
   return ToRange;
 }
 SourceRange ASTImporter::Import(SourceRange FromRange) {
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
-Expected<FileID> ASTImporter::Import_New(FileID FromID) {
-  FileID ToID = Import(FromID);
+Expected<FileID> ASTImporter::Import_New(FileID FromID, bool IsBuiltin) {
+  FileID ToID = Import(FromID, IsBuiltin);
   if (ToID.isInvalid() && FromID.isValid())
     return llvm::make_error<ImportError>();
   return ToID;
 }
-FileID ASTImporter::Import(FileID FromID) {
+FileID ASTImporter::Import(FileID FromID, bool IsBuiltin) {
   llvm::DenseMap<FileID, FileID>::iterator Pos = ImportedFileIDs.find(FromID);
   if (Pos != ImportedFileIDs.end())
     return Pos->second;
 
   SourceManager &FromSM = FromContext.getSourceManager();
   SourceManager &ToSM = ToContext.getSourceManager();
   const SrcMgr::SLocEntry &FromSLoc = FromSM.getSLocEntry(FromID);
 
if (FromEx.isMacroArgExpansion()) {
       MLoc = ToSM.createMacroArgExpansionLoc(ToSpLoc, ToExLocS, TokenLen);
     } else {
       SourceLocation ToExLocE = Import(FromEx.getExpansionLocEnd());
       MLoc = ToSM.createExpansionLoc(ToSpLoc, ToExLocS, ToExLocE, TokenLen,
                                      FromEx.isExpansionTokenRange());
     }
     ToID = ToSM.getFileID(MLoc);
   } else {
+    const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
+
+    if (!IsBuiltin) {
       // Include location of this file.
       SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
 
-    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());
         // 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());
       }
-    if (ToID.isInvalid()) {
+    }
+
+    if (ToID.isInvalid() || IsBuiltin) {
       // FIXME: We want to re-use the existing MemoryBuffer!
       bool Invalid = true;
       const llvm::MemoryBuffer *FromBuf = Cache->getBuffer(
           FromContext.getDiagnostics(), FromSM, SourceLocation{}, &Invalid);
       if (!FromBuf || Invalid)
         return {};
 
       std::unique_ptr<llvm::MemoryBuffer> ToBuf =