[CrossTU] Fix plist macro expansion if macro in other file.

[balazske]

Summary

When cross TU analysis is used it is possible that a macro expansion
is generated for a macro that is defined (and used) in other than
the main translation unit. To get the expansion for it the source
location in the original source file and original preprocessor
is needed.

[Szelethus]

LGTM! Thanks!

[balazske]

@ilya-biryukov Please check if it is acceptable to use ASTUnit in PlistDiagnostic.cpp.

[ilya-biryukov]

StaticAnalyzer/Core does not depend on clangFrontend now, you can see this by looking at lib/StaticAnalyzer/Core/CMakeLists.txt:

add_clang_library(clangStaticAnalyzerCore
...
  LINK_LIBS
  clangAST
  clangASTMatchers
  clangAnalysis
  clangBasic
  clangCrossTU
  clangLex
  clangRewrite
  )

Not a StaticAnalyzer expert, so I don't know whether it's acceptable to add this dependency to clangStaticAnalyzerCore, you'll have to find someone who owns the code to know whether this dependency is justified.
(My wild guess from looking at the names of the libraries would be that this dependency is not ok and the code should go into clangStaticAnalyzerFrontend instead. But again, not an expert here, just a guess).

But please add a dependency into LINK_LIBS inside CMakeLists.txt if you start depending on clangFrontend.
Most of these violations are found if you build in a cmake -DBUILD_SHARED_LIBS=On configuration.

[martong]

In D64638#1593382, @ilya-biryukov wrote:

StaticAnalyzer/Core does not depend on clangFrontend now, you can see this by looking at lib/StaticAnalyzer/Core/CMakeLists.txt:

add_clang_library(clangStaticAnalyzerCore
...
  LINK_LIBS
  clangAST
  clangASTMatchers
  clangAnalysis
  clangBasic
  clangCrossTU
  clangLex
  clangRewrite
  )

Not a StaticAnalyzer expert, so I don't know whether it's acceptable to add this dependency to clangStaticAnalyzerCore, you'll have to find someone who owns the code to know whether this dependency is justified.
(My wild guess from looking at the names of the libraries would be that this dependency is not ok and the code should go into clangStaticAnalyzerFrontend instead. But again, not an expert here, just a guess).

But please add a dependency into LINK_LIBS inside CMakeLists.txt if you start depending on clangFrontend.
Most of these violations are found if you build in a cmake -DBUILD_SHARED_LIBS=On configuration.

@Szelethus @NoQ Could StaticAnalyzer/Core depend on clangFrontend? I am not sure if we can get the Preprocessor somewhere else ...

[balazske]

It is possible to use the Preprocessor instead of ASTUnit in getImportedFromSourceLocation. But this will contain less useful (even if currently not used) information. The test extension in D65064 is not possible to do if not ASTUnit is used. (Or a tuple must be used that contains Preprocessor and TranslationUnitDecl and maybe other objects that are stored in the ASTUnit.)

[balazske]

Maybe CrossTU should not use the Frontend library? (Now the StaticAnalyzerCore is using CrossTU and CrossTU is using Frontend, in this way StaticAnalyzerCore could use Frontend too.)

[NoQ]

The preprocessor is defined in Lex, so i guess i'm curious about a more precise description of what's impossible to do without Frontend.

[balazske]

The Frontend is needed because "ASTUnit.h" is added now into PlistDiagnostics.cpp. The ASTUnit object is used to pass information out from getImportedFromSourceLocation. The StaticAnalyzerCore library uses CrossTU library, and CrossTU uses Frontend, so I think it may be not a problem if StaticAnalyzerCore will use directly Frontend. Otherwise the getImportedFromSourceLocation can return the Preprocessor (and ASTContext) (in a tuple) instead of an ASTUnit and use of ASTUnit is not needed. (Patch in D65064 contains a test that does work only if the TranslationUnitDecl of the imported-from AST is known, to get this value an ASTUnit or ASTContext must be returned from getImportedFromSourceLocation.)

[balazske]

Added clangFrontend to CMakeLists.txt.

[RKSimon]

@balazske This is causing buildbot failures - revert?

[balazske]

Problem should be fixed now (r367013).

cfe/trunk/lib/StaticAnalyzer/Core/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCore
   WorkList.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangAnalysis
   clangBasic
   clangCrossTU
+  clangFrontend
   clangLex
   clangRewrite
   )
 

cfe/trunk/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/PlistSupport.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/Version.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Frontend/ASTUnit.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/TokenConcatenation.h"
 #include "clang/Rewrite/Core/HTMLRewrite.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
 #include "clang/StaticAnalyzer/Core/IssueHash.h"
 #include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
 #include "llvm/ADT/SmallPtrSet.h"

 
 namespace {
 
 /// A helper class for emitting a single report.
 class PlistPrinter {
   const FIDMap& FM;
   AnalyzerOptions &AnOpts;
   const Preprocessor &PP;
+  const cross_tu::CrossTranslationUnitContext &CTU;
   llvm::SmallVector<const PathDiagnosticMacroPiece *, 0> MacroPieces;
 
 public:
   PlistPrinter(const FIDMap& FM, AnalyzerOptions &AnOpts,
-               const Preprocessor &PP)
-    : FM(FM), AnOpts(AnOpts), PP(PP) {
+               const Preprocessor &PP,
+               const cross_tu::CrossTranslationUnitContext &CTU)
+    : FM(FM), AnOpts(AnOpts), PP(PP), CTU(CTU) {
   }
 
   void ReportDiag(raw_ostream &o, const PathDiagnosticPiece& P) {
     ReportPiece(o, P, /*indent*/ 4, /*depth*/ 0, /*includeControlFlow*/ true);
 
     // Don't emit a warning about an unused private field.
     (void)AnOpts;
   }
struct ExpansionInfo {
   std::string Expansion;
   ExpansionInfo(std::string N, std::string E)
     : MacroName(std::move(N)), Expansion(std::move(E)) {}
 };
 
 } // end of anonymous namespace
 
 static void printBugPath(llvm::raw_ostream &o, const FIDMap& FM,
-                         AnalyzerOptions &AnOpts,
-                         const Preprocessor &PP,
+                         AnalyzerOptions &AnOpts, const Preprocessor &PP,
+                         const cross_tu::CrossTranslationUnitContext &CTU,
                          const PathPieces &Path);
 
 /// Print coverage information to output stream {@code o}.
 /// May modify the used list of files {@code Fids} by inserting new ones.
 static void printCoverage(const PathDiagnostic *D,
                           unsigned InputIndentLevel,
                           SmallVectorImpl<FileID> &Fids,
                           FIDMap &FM,
                           llvm::raw_fd_ostream &o);
 
-static ExpansionInfo getExpandedMacro(SourceLocation MacroLoc,
-                                      const Preprocessor &PP);
+static ExpansionInfo
+getExpandedMacro(SourceLocation MacroLoc, const Preprocessor &PP,
+                 const cross_tu::CrossTranslationUnitContext &CTU);
 
 //===----------------------------------------------------------------------===//
 // Methods of PlistPrinter.
 //===----------------------------------------------------------------------===//
 
 void PlistPrinter::EmitRanges(raw_ostream &o,
                               const ArrayRef<SourceRange> Ranges,
                               unsigned indent) {
for (PathPieces::const_iterator I = P.subPieces.begin(),
     ReportPiece(o, **I, indent, depth, /*includeControlFlow*/ false);
   }
 }
 
 void PlistPrinter::ReportMacroExpansions(raw_ostream &o, unsigned indent) {
 
   for (const PathDiagnosticMacroPiece *P : MacroPieces) {
     const SourceManager &SM = PP.getSourceManager();
-    ExpansionInfo EI = getExpandedMacro(P->getLocation().asLocation(), PP);
+    ExpansionInfo EI = getExpandedMacro(P->getLocation().asLocation(), PP, CTU);
 
     Indent(o, indent) << "<dict>\n";
     ++indent;
 
     // Output the location.
     FullSourceLoc L = P->getLocation().asLocation();
 
     Indent(o, indent) << "<key>location</key>\n";
static void printCoverage(const PathDiagnostic *D,
   }
   IndentLevel--;
   Indent(o, IndentLevel) << "</dict>\n";
 
   assert(IndentLevel == InputIndentLevel);
 }
 
 static void printBugPath(llvm::raw_ostream &o, const FIDMap& FM,
-                         AnalyzerOptions &AnOpts,
-                         const Preprocessor &PP,
+                         AnalyzerOptions &AnOpts, const Preprocessor &PP,
+                         const cross_tu::CrossTranslationUnitContext &CTU,
                          const PathPieces &Path) {
-  PlistPrinter Printer(FM, AnOpts, PP);
+  PlistPrinter Printer(FM, AnOpts, PP, CTU);
   assert(std::is_partitioned(
            Path.begin(), Path.end(),
            [](const std::shared_ptr<PathDiagnosticPiece> &E)
              { return E->getKind() == PathDiagnosticPiece::Note; }) &&
          "PathDiagnostic is not partitioned so that notes precede the rest");
 
   PathPieces::const_iterator FirstNonNote = std::partition_point(
       Path.begin(), Path.end(),
o << " <key>diagnostics</key>\n"
        " <array>\n";
 
   for (std::vector<const PathDiagnostic*>::iterator DI=Diags.begin(),
        DE = Diags.end(); DI!=DE; ++DI) {
 
     o << "  <dict>\n";
 
     const PathDiagnostic *D = *DI;
-    printBugPath(o, FM, AnOpts, PP, D->path);
+    printBugPath(o, FM, AnOpts, PP, CTU, D->path);
 
     // Output the bug type and bug category.
     o << "   <key>description</key>";
     EmitString(o, D->getShortDescription()) << '\n';
     o << "   <key>category</key>";
     EmitString(o, D->getCategory()) << '\n';
     o << "   <key>type</key>";
     EmitString(o, D->getBugType()) << '\n';
static const MacroInfo *getMacroInfoForLocation(const Preprocessor &PP,
                                                 const SourceManager &SM,
                                                 const IdentifierInfo *II,
                                                 SourceLocation Loc);
 
 //===----------------------------------------------------------------------===//
 // Definitions of helper functions and methods for expanding macros.
 //===----------------------------------------------------------------------===//
 
-static ExpansionInfo getExpandedMacro(SourceLocation MacroLoc,
-                                      const Preprocessor &PP) {
+static ExpansionInfo
+getExpandedMacro(SourceLocation MacroLoc, const Preprocessor &PP,
+                 const cross_tu::CrossTranslationUnitContext &CTU) {
+
+  const Preprocessor *PPToUse = &PP;
+  if (auto LocAndUnit = CTU.getImportedFromSourceLocation(MacroLoc)) {
+    MacroLoc = LocAndUnit->first;
+    PPToUse = &LocAndUnit->second->getPreprocessor();
+  }
 
   llvm::SmallString<200> ExpansionBuf;
   llvm::raw_svector_ostream OS(ExpansionBuf);
-  TokenPrinter Printer(OS, PP);
+  TokenPrinter Printer(OS, *PPToUse);
   llvm::SmallPtrSet<IdentifierInfo*, 8> AlreadyProcessedTokens;
 
-  std::string MacroName =
-            getMacroNameAndPrintExpansion(Printer, MacroLoc, PP, MacroArgMap{},
-                                         AlreadyProcessedTokens);
+  std::string MacroName = getMacroNameAndPrintExpansion(
+      Printer, MacroLoc, *PPToUse, MacroArgMap{}, AlreadyProcessedTokens);
   return { MacroName, OS.str() };
 }
 
 static std::string getMacroNameAndPrintExpansion(
     TokenPrinter &Printer,
     SourceLocation MacroLoc,
     const Preprocessor &PP,
     const MacroArgMap &PrevArgs,

cfe/trunk/test/Analysis/Inputs/plist-macros-ctu.h

+#define M_H *A = (int *)0
+void F_H(int **A) {
+  M_H;
+}

cfe/trunk/test/Analysis/Inputs/plist-macros-ctu.c

+
+#include "plist-macros-ctu.h"
+
+#define M *X = (int *)0
+
+void F1(int **X) {
+  M;
+}
+
+#undef M
+#define M *Y = (int *)0
+
+void F2(int **Y) {
+  M;
+}
+
+#define M1 *Z = (int *)0
+
+void F3(int **Z) {
+  M1;
+}

cfe/trunk/test/Analysis/Inputs/plist-macros-with-expansion-ctu.c.externalDefMap.txt

+c:@F@F1 plist-macros-ctu.c.ast
+c:@F@F2 plist-macros-ctu.c.ast
+c:@F@F3 plist-macros-ctu.c.ast
+c:@F@F_H plist-macros-ctu.c.ast

cfe/trunk/test/Analysis/plist-macros-with-expansion-ctu.c

+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu \
+// RUN:   -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
+
+// 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>
+
+
+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>
+
+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>
+
+#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>
+
+#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>
+
+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>