[ASTImporter] import macro source locations

[r.stahl]

Summary

Implement full import of macro expansion info with spelling and expansion locations.

[r.stahl]

The split-token case should be covered by this, because it takes the correct TokenLen and the isTokenRange flag. Other than that the split-token ExpansionInfo is indistinguishable.

What about loaded source locations? Do they need to be handled specifically or does this cover everything now?

[r.stahl]

remove stray include

[martong]

This patch is really useful and LGTM!
Just found some minor things.

lib/AST/ASTImporter.cpp

7193

Let's say we import a SourceLocation with ASTImporter::Import(SourceLocation FromLoc).
That calls into ASTImporter::Import(FileID FromID) where we again import other source locations.
Could the initial FromLoc be equal with any of these locations (FromEx.getSpellingLoc(), FromEx.getExpansionLocStart()) ?
My understanding is that this is not possible because we cannot have recursive macros, but please confirm.

unittests/AST/ASTImporterTest.cpp

1580

Perhaps it would be more concise and less error prone to use a FullSourceLoc which wraps one simple SourceLocation and a SourceManager.

1616

CompareSourceRanges(FullSourceRange{ToD->getSourceRange(), ToSM}, ..

?

[r.stahl]

lib/AST/ASTImporter.cpp

7193

Yes, that was my understanding as well. If some compiler error is a macro expansion it eventually stops at some point while walking this chain.

unittests/AST/ASTImporterTest.cpp

1580

Will do, thanks!

[r.stahl]

addressed review comment, but there is nothing like FullSourceRange to improve everything

[martong]

Adding @balazske and @xazax.hun as reviewers. I think if it gets one more approve then we could merge. I'd like to speed up the things here ... we can't expect Aleksei to review all those many patches we sent lately.

[balazske]

unittests/AST/ASTImporterTest.cpp

1593

This test causes every case for expansion (macro, macro arg) to be executed at import?

[thakis]

This code is live when reading pchs, correct? Does this have any measurable perf impact on deserializing pchs for, say, Cocoa.h or Windows.h?

[r.stahl]

In D47698#1140629, @thakis wrote:

This code is live when reading pchs, correct? Does this have any measurable perf impact on deserializing pchs for, say, Cocoa.h or Windows.h?

I don't know for sure, but it should be used - yes. I have not measured a possible performance impact. Do you have a suggestion how I could do this on a Linux setup?

Note that I did not implement this as nice-to-have feature, but for fixing a concrete issue: https://reviews.llvm.org/D26054#1085413

unittests/AST/ASTImporterTest.cpp

1593

Yes, the last DeclRef will be "arg" on the RHS which is a macro argument expansion and the last IntegerLiteral will be the "1" which is a non-argument macro expansion.

[martong]

In D47698#1140629, @thakis wrote:

This code is live when reading pchs, correct? Does this have any measurable perf impact on deserializing pchs for, say, Cocoa.h or Windows.h?

No. The deserialization of a PCH is handled in a completely independent source code (e.g. ASTUnit::LoadFromASTFile). ASTImporter is responsible to merge two ASTs. These ASTs may come from a PCH - deserialized - , or they may be provided by the parser; from the ASTImporer's point of view this is irrelevant. Thus, there is no performance impact on deserialization of PCHs.

ASTImporter is used in LLDB with the command exec when you want to evaluate an expression and during cross translation unit static analysis.

[a.sidorin]

Hi Rafael,

I apologize for the delay in review and hope to get ASTImporter patches reviewed on this weekend.

lib/AST/ASTImporter.cpp

7188–7189

'for to' => 'for the "from" source manager to'?

[a.sidorin]

Hi Rafael,

I think the patch is great. But, honestly, I have never dealt with SourceLocation machinery closely, so some things are a bit unclear to me.

lib/AST/ASTImporter.cpp

7193

If we have a macro referring another macro in the same file, will we import their FileID twice? First, during Import(getSpellingLoc()) and then in this caller?

7195

ToExLocE seems to be unused in the true branch below. Could we move it into 'else' branch?

unittests/AST/ASTImporterTest.cpp

1596

Could you please add a test with nested macros? I.e.

#define FUNC_INT void declToImport
#define FUNC FUNC_INT
FUNC(int a);

[r.stahl]

improved code quality; added nested macro test. it "works", but is disabled because it revealed another bug: the function end location is not imported. will send a patch

[r.stahl]

lib/AST/ASTImporter.cpp

7193

That is taken care of by the ImportedFileIDs check.

lib/AST/ASTImporter.cpp

TemplateName ASTImporter::Import(TemplateName From) {
   llvm_unreachable("Invalid template name kind");
 }
 
 SourceLocation ASTImporter::Import(SourceLocation FromLoc) {
   if (FromLoc.isInvalid())
     return {};
 
   SourceManager &FromSM = FromContext.getSourceManager();
-  
-  // For now, map everything down to its file location, so that we
-  // don't have to import macro expansions.
-  // FIXME: Import macro expansions!
-  FromLoc = FromSM.getFileLoc(FromLoc);
   std::pair<FileID, unsigned> Decomposed = FromSM.getDecomposedLoc(FromLoc);
-  SourceManager &ToSM = ToContext.getSourceManager();
   FileID ToFileID = Import(Decomposed.first);
   if (ToFileID.isInvalid())
     return {};
-  SourceLocation ret = ToSM.getLocForStartOfFile(ToFileID)
-                           .getLocWithOffset(Decomposed.second);
-  return ret;
+  SourceManager &ToSM = ToContext.getSourceManager();
+  return ToSM.getComposedLoc(ToFileID, Decomposed.second);
 }
 
 SourceRange ASTImporter::Import(SourceRange FromRange) {
   return SourceRange(Import(FromRange.getBegin()), Import(FromRange.getEnd()));
 }
 
 FileID ASTImporter::Import(FileID FromID) {
-  llvm::DenseMap<FileID, FileID>::iterator Pos
-    = ImportedFileIDs.find(FromID);
+  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);
-  assert(FromSLoc.isFile() && "Cannot handle macro expansions yet");
-  
+  // Map the FromID to the "to" source manager.
+  FileID ToID;
+  if (FromSLoc.isExpansion()) {
+    const SrcMgr::ExpansionInfo &FromEx = FromSLoc.getExpansion();
+    SourceLocation ToSpLoc = Import(FromEx.getSpellingLoc());
+    SourceLocation ToExLocS = Import(FromEx.getExpansionLocStart());
+    unsigned TokenLen = FromSM.getFileIDSize(FromID);
+    SourceLocation MLoc;
+    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 {
     // Include location of this file.
     SourceLocation ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
-  
-  // Map the FileID for to the "to" source manager.
-  FileID ToID;
     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());
+      const FileEntry *Entry =
+          ToFileManager.getFile(Cache->OrigEntry->getName());
       if (!Entry)
         return {};
-    ToID = ToSM.createFileID(Entry, ToIncludeLoc, 
+      ToID = ToSM.createFileID(Entry, ToIncludeLoc,
                                FromSLoc.getFile().getFileCharacteristic());
     } else {
       // FIXME: We want to re-use the existing MemoryBuffer!
-    const llvm::MemoryBuffer *
-        FromBuf = Cache->getBuffer(FromContext.getDiagnostics(), FromSM);
-    std::unique_ptr<llvm::MemoryBuffer> ToBuf
-      = llvm::MemoryBuffer::getMemBufferCopy(FromBuf->getBuffer(),
+      const llvm::MemoryBuffer *FromBuf =
+          Cache->getBuffer(FromContext.getDiagnostics(), FromSM);
+      std::unique_ptr<llvm::MemoryBuffer> ToBuf =
+          llvm::MemoryBuffer::getMemBufferCopy(FromBuf->getBuffer(),
                                                FromBuf->getBufferIdentifier());
       ToID = ToSM.createFileID(std::move(ToBuf),
                                FromSLoc.getFile().getFileCharacteristic());
     }
+  }
 
   ImportedFileIDs[FromID] = ToID;
   return ToID;
 }
 
 CXXCtorInitializer *ASTImporter::Import(CXXCtorInitializer *From) {
   Expr *ToExpr = Import(From->getInit());
   if (!ToExpr && From->getInit())

unittests/AST/ASTImporterTest.cpp

auto *FromD = FirstDeclMatcher<CXXRecordDecl>().match(
       FromTU, cxxRecordDecl(hasName("B")));
 
   Import(FromD, Lang_CXX);
 
   EXPECT_EQ(2u, DeclCounter<CXXRecordDecl>().match(
                     ToTU, cxxRecordDecl(unless(isImplicit()))));
 }
 
+static void CompareSourceLocs(FullSourceLoc Loc1, FullSourceLoc Loc2) {
+  EXPECT_EQ(Loc1.getExpansionLineNumber(), Loc2.getExpansionLineNumber());
+  EXPECT_EQ(Loc1.getExpansionColumnNumber(), Loc2.getExpansionColumnNumber());
+  EXPECT_EQ(Loc1.getSpellingLineNumber(), Loc2.getSpellingLineNumber());
+  EXPECT_EQ(Loc1.getSpellingColumnNumber(), Loc2.getSpellingColumnNumber());
+}
+static void CompareSourceRanges(SourceRange Range1, SourceRange Range2,
+                                SourceManager &SM1, SourceManager &SM2) {
+  CompareSourceLocs(FullSourceLoc{ Range1.getBegin(), SM1 },
+                    FullSourceLoc{ Range2.getBegin(), SM2 });
+  CompareSourceLocs(FullSourceLoc{ Range1.getEnd(), SM1 },
+                    FullSourceLoc{ Range2.getEnd(), SM2 });
+}
+TEST_P(ASTImporterTestBase, ImportSourceLocs) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      #define MFOO(arg) arg = arg + 1
+
+      void foo() {
+        int a = 5;
+        MFOO(a);
+      }
+      )",
+      Lang_CXX);
+  auto FromD = FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());
+  auto ToD = Import(FromD, Lang_CXX);
+
+  auto ToLHS = LastDeclMatcher<DeclRefExpr>().match(ToD, declRefExpr());
+  auto FromLHS = LastDeclMatcher<DeclRefExpr>().match(FromTU, declRefExpr());
+  auto ToRHS = LastDeclMatcher<IntegerLiteral>().match(ToD, integerLiteral());
+  auto FromRHS =
+      LastDeclMatcher<IntegerLiteral>().match(FromTU, integerLiteral());
+
+  SourceManager &ToSM = ToAST->getASTContext().getSourceManager();
+  SourceManager &FromSM = FromD->getASTContext().getSourceManager();
+  CompareSourceRanges(ToD->getSourceRange(), FromD->getSourceRange(), ToSM,
+                      FromSM);
+  CompareSourceRanges(ToLHS->getSourceRange(), FromLHS->getSourceRange(), ToSM,
+                      FromSM);
+  CompareSourceRanges(ToRHS->getSourceRange(), FromRHS->getSourceRange(), ToSM,
+                      FromSM);
+}
+
+TEST_P(ASTImporterTestBase, DISABLED_ImportNestedMacro) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      #define FUNC_INT void declToImport
+      #define FUNC FUNC_INT
+      FUNC(int a);
+      )",
+      Lang_CXX);
+  auto FromD = FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());
+  auto ToD = Import(FromD, Lang_CXX);
+
+  SourceManager &ToSM = ToAST->getASTContext().getSourceManager();
+  SourceManager &FromSM = FromD->getASTContext().getSourceManager();
+  CompareSourceRanges(ToD->getSourceRange(), FromD->getSourceRange(), ToSM,
+                      FromSM);
+}
+
 TEST_P(
     ASTImporterTestBase,
     ImportDefinitionOfClassTemplateSpecIfThereIsAnExistingFwdDeclAndDefinition)
 {
   Decl *ToTU = getToTuDecl(
       R"(
       template <typename T>
       struct B;