[analyzer][CTU] API for CTU macro expansions

[steakhal]

Summary

Removes CrossTranslationUnitContext::getImportedFromSourceLocation
Removes the corresponding unit-test segment.

Introduces the CrossTranslationUnitContext::getMacroExpansionContextForSourceLocation
which will return the macro expansion context for an imported TU. Also adds a
few implementation FIXME notes where applicable, since this feature is
not implemented yet. This fact is also noted as Doxygen comments.

Uplifts a few CTU LIT test to match the current incomplete behavior.

It is a regression to some extent since now we don't expand any
macros in imported TUs. At least we don't crash anymore.

Note that the introduced function is already covered by LIT tests.
Eg.: Analysis/plist-macros-with-expansion-ctu.c

[balazske]

Probably the CrossTranslationUnitContext::ImportedFileIDs can be removed. The FileIDImportHandler in ASTImporter was used only for this (if I know correctly) too but it can be useful if a similar mechanism is needed later, but it remains there as "dead code" if not removed.

[steakhal]

In D94673#2498165, @balazske wrote:

Probably the CrossTranslationUnitContext::ImportedFileIDs can be removed. The FileIDImportHandler in ASTImporter was used only for this (if I know correctly) too but it can be useful if a similar mechanism is needed later, but it remains there as "dead code" if not removed.

Excellent catch!

[steakhal]

Update: resolve @balazske's comment D94673#2498165.

• remove ASTImporter::FileIDImportHandler etc.
• updates the revision's summary accordingly

[vsavchenko]

clang/include/clang/CrossTU/CrossTranslationUnit.h

186–187

I think it's missing "location" here

188–189

I don't think that this piece of information should be here, it took me a bit to get it. Maybe it can be a separate \note?

[steakhal]

Updated the comment of CrossTranslationUnitContext::getMacroExpansionContextForSourceLocation.

[steakhal]

clang/include/clang/CrossTU/CrossTranslationUnit.h

191

is

[steakhal]

Actually, somehow I messed up something previously. Now, the diff contains the latest version.
Sorry for the inconvenience!

Rebased on top of 2407eb08a5748bc2613e95fa449fc1cae6f4ff8f.

[Szelethus]

LGTM! You may wanna wait for someone more knowledgeable about CTU to give it another accept.

[balazske]

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

835

I am not sure if getExpandedText will handle a source location that is not in the same TU. Probably the previous source location mapping mechanism (that is now removed) is needed additionally, so that here the original source location and the original MacroExpansionContext is available. Probably this can be done in other way, the MacroExpansionContext could handle this mapping. (A single MacroExpansionContext could handle every imported source location at least for on-demand parsing.)

[steakhal]

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

835

Yes. For a complete CTU implementation, it seems inevitable to be able to acquire the proper MacroExpansionContext for an imported source-location. So the previous mapping will be likely back once we implement this functionality completely.

I think we can not implement it via having a single MacroExpansionContext due to layering violations.
I wanted this to be part of the Analysis library, but if it was concerned about CTU importing and stuff it would reside inside the cross_tu library.
I might be wrong about this though.

clang/include/clang/AST/ASTImporter.h

class TypeSourceInfo;
   /// Imports selected nodes from one AST context into another context,
   /// merging AST nodes where appropriate.
   class ASTImporter {
     friend class ASTNodeImporter;
   public:
     using NonEquivalentDeclSet = llvm::DenseSet<std::pair<Decl *, Decl *>>;
     using ImportedCXXBaseSpecifierMap =
         llvm::DenseMap<const CXXBaseSpecifier *, CXXBaseSpecifier *>;
-    using FileIDImportHandlerType =
-        std::function<void(FileID /*ToID*/, FileID /*FromID*/)>;
 
     enum class ODRHandlingType { Conservative, Liberal };
 
     // An ImportPath is the list of the AST nodes which we visit during an
     // Import call.
     // If node `A` depends on node `B` then the path contains an `A`->`B` edge.
     // From the call stack of the import functions we can read the very same
     // path.
private:
       VecTy Nodes;
       // Auxiliary container to be able to answer "Do we have a cycle ending
       // at last element?" as fast as possible.
       // We count each Decl's occurrence over the path.
       llvm::SmallDenseMap<Decl *, int, 32> Aux;
     };
 
   private:
-    FileIDImportHandlerType FileIDImportHandler;
-
     std::shared_ptr<ASTImporterSharedState> SharedState = nullptr;
 
     /// The path which we go through during the import of a given AST node.
     ImportPathTy ImportPath;
     /// Sometimes we have to save some part of an import path, so later we can
     /// set up properties to the saved nodes.
     /// We may have several of these import paths associated to one Decl.
     using SavedImportPathsForOneDecl =
public:
     /// If not set then the original C/C++ lookup is used.
     ASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
                 ASTContext &FromContext, FileManager &FromFileManager,
                 bool MinimalImport,
                 std::shared_ptr<ASTImporterSharedState> SharedState = nullptr);
 
     virtual ~ASTImporter();
 
-    /// Set a callback function for FileID import handling.
-    /// The function is invoked when a FileID is imported from the From context.
-    /// The imported FileID in the To context and the original FileID in the
-    /// From context is passed to it.
-    void setFileIDImportHandler(FileIDImportHandlerType H) {
-      FileIDImportHandler = H;
-    }
-
     /// Whether the importer will perform a minimal import, creating
     /// to-be-completed forward declarations when possible.
     bool isMinimalImport() const { return Minimal; }
 
     void setODRHandling(ODRHandlingType T) { ODRHandling = T; }
 
     /// \brief Import the given object, returns the result.
     ///

clang/include/clang/CrossTU/CrossTranslationUnit.h

 //  feature can be utilized for tools that require cross translation unit
 //  support.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_CROSSTU_CROSSTRANSLATIONUNIT_H
 #define LLVM_CLANG_CROSSTU_CROSSTRANSLATIONUNIT_H
 
 #include "clang/AST/ASTImporterSharedState.h"
+#include "clang/Analysis/MacroExpansionContext.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 
llvm::Expected<const VarDecl *> importDefinition(const VarDecl *VD,
                                                    ASTUnit *Unit);
 
   /// Get a name to identify a named decl.
   static llvm::Optional<std::string> getLookupName(const NamedDecl *ND);
 
   /// Emit diagnostics for the user for potential configuration errors.
   void emitCrossTUDiagnostics(const IndexError &IE);
 
-  /// Determine the original source location in the original TU for an
-  /// imported source location.
+  /// Returns the MacroExpansionContext for the imported TU to which the given
+  /// source-location corresponds.
   /// \p ToLoc Source location in the imported-to AST.
-  /// \return Source location in the imported-from AST and the corresponding
-  /// ASTUnit object (the AST was loaded from a file using an internal ASTUnit
-  /// object that is returned here).
-  /// If any error happens (ToLoc is a non-imported source location) empty is
-  /// returned.
-  llvm::Optional<std::pair<SourceLocation /*FromLoc*/, ASTUnit *>>
-  getImportedFromSourceLocation(const clang::SourceLocation &ToLoc) const;
+  /// \note If any error happens such as \p ToLoc is a non-imported
+  ///       source-location, empty is returned.
+  /// \note Macro expansion tracking for imported TUs is not implemented yet.
+  ///       It returns empty unconditionally.
+  llvm::Optional<clang::MacroExpansionContext>
+  getMacroExpansionContextForSourceLocation(
+      const clang::SourceLocation &ToLoc) const;
 
 private:
-  using ImportedFileIDMap =
-      llvm::DenseMap<FileID, std::pair<FileID, ASTUnit *>>;
-
   void lazyInitImporterSharedSt(TranslationUnitDecl *ToTU);
   ASTImporter &getOrCreateASTImporter(ASTUnit *Unit);
   template <typename T>
   llvm::Expected<const T *> getCrossTUDefinitionImpl(const T *D,
                                                      StringRef CrossTUDir,
                                                      StringRef IndexName,
                                                      bool DisplayCTUProgress);
   template <typename T>
   const T *findDefInDeclContext(const DeclContext *DC,
                                 StringRef LookupName);
   template <typename T>
   llvm::Expected<const T *> importDefinitionImpl(const T *D, ASTUnit *Unit);
 
   using ImporterMapTy =
       llvm::DenseMap<TranslationUnitDecl *, std::unique_ptr<ASTImporter>>;
 
   ImporterMapTy ASTUnitImporterMap;
 
   ASTContext &Context;
   std::shared_ptr<ASTImporterSharedState> ImporterSharedSt;
-  /// Map of imported FileID's (in "To" context) to FileID in "From" context
-  /// and the ASTUnit for the From context.
-  /// This map is used by getImportedFromSourceLocation to lookup a FileID and
-  /// its Preprocessor when knowing only the FileID in the 'To' context. The
-  /// FileID could be imported by any of multiple 'From' ASTImporter objects.
-  /// we do not want to loop over all ASTImporter's to find the one that
-  /// imported the FileID.
-  ImportedFileIDMap ImportedFileIDs;
 
   using LoadResultTy = llvm::Expected<std::unique_ptr<ASTUnit>>;
 
   /// Loads ASTUnits from AST-dumps or source-files.
   class ASTLoader {
   public:
     ASTLoader(CompilerInstance &CI, StringRef CTUDir,
               StringRef InvocationListFilePath);

clang/lib/AST/ASTImporter.cpp

if (ToID.isInvalid() || IsBuiltin) {
       ToID = ToSM.createFileID(std::move(ToBuf),
                                FromSLoc.getFile().getFileCharacteristic());
     }
   }
 
   assert(ToID.isValid() && "Unexpected invalid fileID was created.");
 
   ImportedFileIDs[FromID] = ToID;
-
-  if (FileIDImportHandler)
-    FileIDImportHandler(ToID, FromID);
-
   return ToID;
 }
 
 Expected<CXXCtorInitializer *> ASTImporter::Import(CXXCtorInitializer *From) {
   ExpectedExpr ToExprOrErr = Import(From->getInit());
   if (!ToExprOrErr)
     return ToExprOrErr.takeError();
 

clang/lib/CrossTU/CrossTranslationUnit.cpp

CrossTranslationUnitContext::getOrCreateASTImporter(ASTUnit *Unit) {
 
   auto I = ASTUnitImporterMap.find(From.getTranslationUnitDecl());
   if (I != ASTUnitImporterMap.end())
     return *I->second;
   lazyInitImporterSharedSt(Context.getTranslationUnitDecl());
   ASTImporter *NewImporter = new ASTImporter(
       Context, Context.getSourceManager().getFileManager(), From,
       From.getSourceManager().getFileManager(), false, ImporterSharedSt);
-  NewImporter->setFileIDImportHandler([this, Unit](FileID ToID, FileID FromID) {
-    assert(ImportedFileIDs.find(ToID) == ImportedFileIDs.end() &&
-           "FileID already imported, should not happen.");
-    ImportedFileIDs[ToID] = std::make_pair(FromID, Unit);
-  });
   ASTUnitImporterMap[From.getTranslationUnitDecl()].reset(NewImporter);
   return *NewImporter;
 }
 
-llvm::Optional<std::pair<SourceLocation, ASTUnit *>>
-CrossTranslationUnitContext::getImportedFromSourceLocation(
+llvm::Optional<clang::MacroExpansionContext>
+CrossTranslationUnitContext::getMacroExpansionContextForSourceLocation(
     const clang::SourceLocation &ToLoc) const {
-  const SourceManager &SM = Context.getSourceManager();
-  auto DecToLoc = SM.getDecomposedLoc(ToLoc);
-
-  auto I = ImportedFileIDs.find(DecToLoc.first);
-  if (I == ImportedFileIDs.end())
-    return {};
-
-  FileID FromID = I->second.first;
-  clang::ASTUnit *Unit = I->second.second;
-  SourceLocation FromLoc =
-      Unit->getSourceManager().getComposedLoc(FromID, DecToLoc.second);
-
-  return std::make_pair(FromLoc, Unit);
+  // FIXME: Implement: Record such a context for every imported ASTUnit; lookup.
+  return llvm::None;
 }
 
 } // namespace cross_tu
 } // namespace clang

clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

 // Definitions of helper functions and methods for expanding macros.
 //===----------------------------------------------------------------------===//
 
 static Optional<StringRef>
 getExpandedMacro(SourceLocation MacroExpansionLoc,
                  const cross_tu::CrossTranslationUnitContext &CTU,
                  const MacroExpansionContext &MacroExpansions,
                  const SourceManager &SM) {
-  if (auto LocAndUnit = CTU.getImportedFromSourceLocation(MacroExpansionLoc)) {
-    // TODO: Implement macro expansions for CTU.
-    return llvm::None;
+  if (auto CTUMacroExpCtx =
+          CTU.getMacroExpansionContextForSourceLocation(MacroExpansionLoc)) {
+    return CTUMacroExpCtx->getExpandedText(MacroExpansionLoc);
   }
   return MacroExpansions.getExpandedText(MacroExpansionLoc);
 }

clang/test/Analysis/plist-macros-with-expansion-ctu.c

 // RUN: rm -rf %t && mkdir %t
 // RUN: mkdir -p %t/ctudir
 // RUN: %clang_cc1 -emit-pch -o %t/ctudir/plist-macros-ctu.c.ast %S/Inputs/plist-macros-ctu.c
 // RUN: cp %S/Inputs/plist-macros-with-expansion-ctu.c.externalDefMap.txt %t/ctudir/externalDefMap.txt
-
+//
 // RUN: %clang_analyze_cc1 -analyzer-checker=core \
 // RUN:   -analyzer-config experimental-enable-naive-ctu-analysis=true \
 // RUN:   -analyzer-config ctu-dir=%t/ctudir \
 // RUN:   -analyzer-config expand-macros=true \
 // RUN:   -analyzer-output=plist-multi-file -o %t.plist -verify %s
-// XFAIL: *
+//
 // Check the macro expansions from the plist output here, to make the test more
 // understandable.
 //   RUN: FileCheck --input-file=%t.plist %s
 
 extern void F1(int **);
 extern void F2(int **);
 extern void F3(int **);
 extern void F_H(int **);
 
 void test0() {
   int *X;
   F3(&X);
   *X = 1; // expected-warning{{Dereference of null pointer}}
 }
-// CHECK: <key>name</key><string>M1</string>
-// CHECK-NEXT: <key>expansion</key><string>*Z = (int *)0</string>
-
+// FIXME: Macro expansion for other TUs should also work.
+// CHECK:      <key>macro_expansions</key>
+// CHECK-NEXT: <array>
+// CHECK-NEXT: </array>
 
 void test1() {
   int *X;
   F1(&X);
   *X = 1; // expected-warning{{Dereference of null pointer}}
 }
-// CHECK: <key>name</key><string>M</string>
-// CHECK-NEXT: <key>expansion</key><string>*X = (int *)0</string>
+
+// CHECK:      <key>macro_expansions</key>
+// CHECK-NEXT: <array>
+// CHECK-NEXT: </array>
 
 void test2() {
   int *X;
   F2(&X);
   *X = 1; // expected-warning{{Dereference of null pointer}}
 }
-// CHECK: <key>name</key><string>M</string>
-// CHECK-NEXT: <key>expansion</key><string>*Y = (int *)0</string>
+
+// CHECK:      <key>macro_expansions</key>
+// CHECK-NEXT: <array>
+// CHECK-NEXT: </array>
 
 #define M F1(&X)
 
 void test3() {
   int *X;
   M;
   *X = 1; // expected-warning{{Dereference of null pointer}}
 }
-// CHECK: <key>name</key><string>M</string>
-// CHECK-NEXT: <key>expansion</key><string>F1(&amp;X)</string>
-// CHECK: <key>name</key><string>M</string>
-// CHECK-NEXT: <key>expansion</key><string>*X = (int *)0</string>
+// Macro expansions for the main TU still works, even in CTU mode.
+// CHECK:      <key>macro_expansions</key>
+// CHECK-NEXT: <array>
+// CHECK-NEXT:  <dict>
+// CHECK-NEXT:   <key>location</key>
+// CHECK-NEXT:   <dict>
+// CHECK-NEXT:    <key>line</key><integer>55</integer>
+// CHECK-NEXT:    <key>col</key><integer>3</integer>
+// CHECK-NEXT:    <key>file</key><integer>0</integer>
+// CHECK-NEXT:   </dict>
+// CHECK-NEXT:   <key>name</key><string>M</string>
+// CHECK-NEXT:   <key>expansion</key><string>F1 (&amp;X )</string>
+// CHECK-NEXT:  </dict>
+// CHECK-NEXT: </array>
 
 #undef M
 #define M F2(&X)
 
 void test4() {
   int *X;
   M;
   *X = 1; // expected-warning{{Dereference of null pointer}}
 }
 
-// CHECK: <key>name</key><string>M</string>
-// CHECK-NEXT: <key>expansion</key><string>F2(&amp;X)</string>
-// CHECK: <key>name</key><string>M</string>
-// CHECK-NEXT: <key>expansion</key><string>*Y = (int *)0</string>
+// CHECK:      <key>macro_expansions</key>
+// CHECK-NEXT: <array>
+// CHECK-NEXT:  <dict>
+// CHECK-NEXT:   <key>location</key>
+// CHECK-NEXT:   <dict>
+// CHECK-NEXT:    <key>line</key><integer>78</integer>
+// CHECK-NEXT:    <key>col</key><integer>3</integer>
+// CHECK-NEXT:    <key>file</key><integer>0</integer>
+// CHECK-NEXT:   </dict>
+// CHECK-NEXT:   <key>name</key><string>M</string>
+// CHECK-NEXT:   <key>expansion</key><string>F2 (&amp;X )</string>
+// CHECK-NEXT:  </dict>
+// CHECK-NEXT: </array>
 
 void test_h() {
   int *X;
   F_H(&X);
   *X = 1; // expected-warning{{Dereference of null pointer}}
 }
 
-// CHECK: <key>name</key><string>M_H</string>
-// CHECK-NEXT: <key>expansion</key><string>*A = (int *)0</string>
+// CHECK:      <key>macro_expansions</key>
+// CHECK-NEXT: <array>
+// CHECK-NEXT: </array>

clang/unittests/CrossTU/CrossTranslationUnitTest.cpp

llvm::Expected<const FunctionDecl *> NewFDorError = handleExpected(
         CTU.getCrossTUDefinition(FD, "", IndexFileName, false),
         []() { return nullptr; }, [](IndexError &) {});
 
     if (NewFDorError) {
       const FunctionDecl *NewFD = *NewFDorError;
       *Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
 
       if (NewFD) {
-        // Check GetImportedFromSourceLocation.
-        llvm::Optional<std::pair<SourceLocation, ASTUnit *>> SLocResult =
-            CTU.getImportedFromSourceLocation(NewFD->getLocation());
-        EXPECT_TRUE(SLocResult);
-        if (SLocResult) {
-          SourceLocation OrigSLoc = (*SLocResult).first;
-          ASTUnit *OrigUnit = (*SLocResult).second;
-          // OrigUnit is created internally by CTU (is not the
-          // ASTWithDefinition).
-          TranslationUnitDecl *OrigTU =
-              OrigUnit->getASTContext().getTranslationUnitDecl();
-          const FunctionDecl *FDWithDefinition = FindFInTU(OrigTU);
-          EXPECT_TRUE(FDWithDefinition);
-          if (FDWithDefinition) {
-            EXPECT_EQ(FDWithDefinition->getName(), "f");
-            EXPECT_TRUE(FDWithDefinition->isThisDeclarationADefinition());
-            EXPECT_EQ(OrigSLoc, FDWithDefinition->getLocation());
-          }
-        }
-
         // Check parent map.
         const DynTypedNodeList ParentsAfterImport =
             Ctx.getParentMapContext().getParents<Decl>(*FD);
         const DynTypedNodeList ParentsOfImported =
             Ctx.getParentMapContext().getParents<Decl>(*NewFD);
         EXPECT_TRUE(
             checkParentListsEq(ParentsBeforeImport, ParentsAfterImport));
         EXPECT_FALSE(ParentsOfImported.empty());