[ASTImporter] Prevent the ASTImporter from creating multiple main FileIDs.

[teemperor]

Summary

When importing the main FileID the ASTImporter currently gives it no include location. This means
that any SourceLocations produced for this FileID look to Clang as if they are coming from the
main FileID (as the main FileID has no include location).

Clang seems to expect that there is only one main FileID in one translation unit (which makes sense
during normal compilation), so this behavior leads to several problems when producing diagnostics,
one being that when calling SourceManager::isBeforeInTranslationUnit on two SourceLocations
that come from two different ASTContext instances, Clang fails to sort the SourceLocations as
the include chains of the FileIDs don't end up in a single FileID. This causes that Clang crashes
with "Unsortable locations found" in this function.

This patch gives any imported main FileIDs the main FileID of the To ASTContext as its include
location. This allows Clang to sort all imported SourceLocations as now all include chains point
to the main FileID of the To ASTContext. The exact include location is currently set to the start
of the To main file (just because that should always be a valid SourceLocation).

[martong]

Looks good to me! Thanks!
Adding @balazske as another reviewer though. He worked on something related to this, where we also experienced an assert in isBeforeInTranslationUnit.

[teemperor]

In D74542#1874201, @martong wrote:

Looks good to me! Thanks!
Adding @balazske as another reviewer though. He worked on something related to this, where we also experienced an assert in isBeforeInTranslationUnit.

IIRC the analyser is using the isBeforeInTranslationUnit for the error paths so that's how the X-TU analysis could hit this situation (if the error path is spanning Decls from different files). But that's mostly speculation.

[martong]

In D74542#1874221, @teemperor wrote:

In D74542#1874201, @martong wrote:

Looks good to me! Thanks!
Adding @balazske as another reviewer though. He worked on something related to this, where we also experienced an assert in isBeforeInTranslationUnit.

IIRC the analyser is using the isBeforeInTranslationUnit for the error paths so that's how the X-TU analysis could hit this situation (if the error path is spanning Decls from different files). But that's mostly speculation.

Yes exactly, but we actually use the SourceLocation from the "From" unit, we have a mapping in CrossTranslationUnitContext for that purpose. See CrossTranslationUnitContext::getImportedFromSourceLocation (and https://reviews.llvm.org/D64554 and https://reviews.llvm.org/D64638)

[a_sidorin]

Nice solution!

[teemperor]

@balazske Do you see any problems with this patch?

[balazske]

In D74542#1878576, @teemperor wrote:

@balazske Do you see any problems with this patch?

I have no opinion against the change. Probably sorting order of imported source locations makes not much sense (specially if recursive imports happen) and I can not tell what is the consequence of this, maybe no problem (there was something that triggered this crash, does this way of sorting order give some usable results?).

[teemperor]

In D74542#1878732, @balazske wrote:

In D74542#1878576, @teemperor wrote:

@balazske Do you see any problems with this patch?

I have no opinion against the change. Probably sorting order of imported source locations makes not much sense (specially if recursive imports happen) and I can not tell what is the consequence of this, maybe no problem (there was something that triggered this crash, does this way of sorting order give some usable results?).

Before this patch we would get a Clang crash in SourceManager::isBeforeInTranslationUnit when emitting diagnostics that cover multiple (main) files. With this patch we get a normal diagnostic. The order of the note: messages is in the order in which they were encountered by LLDB but that is fine as long as the order is deterministic (I also don't see a very logical order in which they should be ordered and we're fine as long as the order is deterministic).

clang/lib/AST/ASTImporter.cpp

if (FromSLoc.isExpansion()) {
     const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();
 
     if (!IsBuiltin) {
       // Include location of this file.
       ExpectedSLoc ToIncludeLoc = Import(FromSLoc.getFile().getIncludeLoc());
       if (!ToIncludeLoc)
         return ToIncludeLoc.takeError();
 
+      // Every FileID that is not the main FileID needs to have a valid include
+      // location so that the include chain points to the main FileID. When
+      // importing the main FileID (which has no include location), we need to
+      // create a fake include location in the main file to keep this property
+      // intact.
+      SourceLocation ToIncludeLocOrFakeLoc = *ToIncludeLoc;
+      if (FromID == FromSM.getMainFileID())
+        ToIncludeLocOrFakeLoc = ToSM.getLocForStartOfFile(ToSM.getMainFileID());
+
       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.
         auto 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,
+          ToID = ToSM.createFileID(*Entry, ToIncludeLocOrFakeLoc,
                                    FromSLoc.getFile().getFileCharacteristic());
       }
     }
 
     if (ToID.isInvalid() || IsBuiltin) {
       // FIXME: We want to re-use the existing MemoryBuffer!
       bool Invalid = true;
       const llvm::MemoryBuffer *FromBuf =

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ImportAutoFunctions, ReturnWithTypeInSwitch) {
   FunctionDecl *From = FirstDeclMatcher<FunctionDecl>().match(
       FromTU, functionDecl(hasName("f")));
 
   FunctionDecl *To = Import(From, Lang_CXX17);
   EXPECT_TRUE(To);
   EXPECT_TRUE(isa<AutoType>(To->getReturnType()));
 }
 
+struct ImportSourceLocations : ASTImporterOptionSpecificTestBase {};
+
+TEST_P(ImportSourceLocations, PreserveFileIDTreeStructure) {
+  // Tests that the FileID tree structure (with the links being the include
+  // chains) is preserved while importing other files (which need to be
+  // added to this structure with fake include locations.
+
+  SourceLocation Location1;
+  {
+    auto Pattern = varDecl(hasName("X"));
+    Decl *FromTU = getTuDecl("int X;", Lang_C, "input0.c");
+    auto *FromD = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern);
+
+    Location1 = Import(FromD, Lang_C)->getLocation();
+  }
+  SourceLocation Location2;
+  {
+    auto Pattern = varDecl(hasName("Y"));
+    Decl *FromTU = getTuDecl("int Y;", Lang_C, "input1.c");
+    auto *FromD = FirstDeclMatcher<VarDecl>().match(FromTU, Pattern);
+
+    Location2 = Import(FromD, Lang_C)->getLocation();
+  }
+
+  SourceManager &ToSM = ToAST->getSourceManager();
+  FileID FileID1 = ToSM.getFileID(Location1);
+  FileID FileID2 = ToSM.getFileID(Location2);
+
+  // Check that the imported files look like as if they were included from the
+  // start of the main file.
+  SourceLocation FileStart = ToSM.getLocForStartOfFile(ToSM.getMainFileID());
+  EXPECT_NE(FileID1, ToSM.getMainFileID());
+  EXPECT_NE(FileID2, ToSM.getMainFileID());
+  EXPECT_EQ(ToSM.getIncludeLoc(FileID1), FileStart);
+  EXPECT_EQ(ToSM.getIncludeLoc(FileID2), FileStart);
+
+  // Let the SourceManager check the order of the locations. The order should
+  // be the order in which the declarations are imported.
+  EXPECT_TRUE(ToSM.isBeforeInTranslationUnit(Location1, Location2));
+  EXPECT_FALSE(ToSM.isBeforeInTranslationUnit(Location2, Location1));
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportImplicitMethods,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportVariables,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, LLDBLookupTest,
                         DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportSourceLocations,
+                        DefaultTestValuesForRunOptions, );
+
 } // end namespace ast_matchers
 } // end namespace clang