[Analyzer][BugReporter] Replace the example bug report with the one used to generate PathDiagnostic

[OikawaKirie]

Summary

In function BugReporter::FlushReport, function PathSensitiveBugReporter::generatePathDiagnostics will re-select one report from the equivalence class to construct the PathDiagnostic. It makes the report used to generate PathDiagnostic may be different from the one used in function BugReporter::FlushReport. Changes to the bug report via a BugReporterVisitor may not be reflected when generating report notes or fix-hints.

This patch tries to fix this problem by using the report used to generate PathDiagnostic directly in function BugReporter::FlushReport to generate notes.

[Szelethus]

First off, your patch is great, and I'm pretty sure we want it!

I remember working around here, and I either have never quite understood what makes exampleReport an example, or have long forgotten. Can we not just rename it to chosenReport, or simply report?

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

3064–3074

Wouldn't it make more sense to create initialize (instead of resetting) report at the call to generateDiagnosticForConsumerMap()? This just looks a bit convoluted. We could turn everything up to that point into a predicate function.

[OikawaKirie]

In D115716#3217786, @Szelethus wrote:

First off, your patch is great, and I'm pretty sure we want it!

I remember working around here, and I either have never quite understood what makes exampleReport an example, or have long forgotten. Can we not just rename it to chosenReport, or simply report?

Thanks a lot.

New updates:

1. add a new BugReporter::generateDiagnosticForConsumerMap method to handle the job before adding notes
2. add a BugReport field to DiagnosticForConsumerMapTy
3. move findReportInEquivalenceClass calls to generateDiagnosticForConsumerMap methods
4. the exampleReport variable in method findReportInEquivalenceClass is unchanged, others are removed as they are no longer required

[Szelethus]

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

76

Some comments about this field would be welcome!

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

3076–3077

Considering that the consumers are already a part of the BugReport, the overload on generateDiagnosticForConsumerMap feels too arbitrary. Wouldn't a predicate function instead of an overload be more fitting?

3210

Ohh, a variable with its pants down!

3286–3290

Really happy to see exampleReport gone as a parameter.

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h

 class Decl;
 class DiagnosticsEngine;
 class LocationContext;
 class SourceManager;
 class Stmt;
 
 namespace ento {
 
+class BugReport;
 class BugType;
 class CheckerBase;
 class ExplodedGraph;
 class ExplodedNode;
 class ExprEngine;
 class MemRegion;
 class SValBuilder;
 
 //===----------------------------------------------------------------------===//
 // Interface for individual bug reports.
 //===----------------------------------------------------------------------===//
 
 /// A mapping from diagnostic consumers to the diagnostics they should
 /// consume.
-using DiagnosticForConsumerMapTy =
-    llvm::DenseMap<PathDiagnosticConsumer *, std::unique_ptr<PathDiagnostic>>;
+struct DiagnosticForConsumerMapTy
+    : llvm::DenseMap<PathDiagnosticConsumer *,
+                     std::unique_ptr<PathDiagnostic>> {
+  const BugReport *Report = nullptr;
+};
 
 /// Interface for classes constructing Stack hints.
 ///
 /// If a PathDiagnosticEvent occurs in a different frame than the final
 /// diagnostic the hints can be used to summarize the effect of the call.
 class StackHintGenerator {
 public:
   virtual ~StackHintGenerator() = 0;
void addNote(StringRef Msg, const PathDiagnosticLocation &Pos,
     auto P = std::make_shared<PathDiagnosticNotePiece>(Pos, Msg);
 
     for (const auto &R : Ranges)
       P->addRange(R);
 
     Notes.push_back(std::move(P));
   }
 
-  ArrayRef<std::shared_ptr<PathDiagnosticNotePiece>> getNotes() {
+  ArrayRef<std::shared_ptr<PathDiagnosticNotePiece>> getNotes() const {
     return Notes;
   }
 
   /// Add a range to a bug report.
   ///
   /// Ranges are used to highlight regions of interest in the source code.
   /// They should be at the same source code line as the BugReport location.
   /// By default, the source range of the statement corresponding to the error
BugType *getBugTypeForName(CheckerNameRef CheckerName, StringRef name,
                              StringRef category);
 
   virtual BugReport *
   findReportInEquivalenceClass(BugReportEquivClass &eqClass,
                                SmallVectorImpl<BugReport *> &bugReports) {
     return eqClass.getReports()[0].get();
   }
 
+  std::unique_ptr<DiagnosticForConsumerMapTy>
+  generateDiagnosticForConsumerMap(BugReportEquivClass &EQ);
+
 protected:
   /// Generate the diagnostics for the given bug report.
   virtual std::unique_ptr<DiagnosticForConsumerMapTy>
-  generateDiagnosticForConsumerMap(BugReport *exampleReport,
-                                   ArrayRef<PathDiagnosticConsumer *> consumers,
-                                   ArrayRef<BugReport *> bugReports);
+  generateDiagnosticForConsumerMap(
+      BugReportEquivClass &EQ, ArrayRef<PathDiagnosticConsumer *> consumers);
 };
 
 /// GRBugReporter is used for generating path-sensitive reports.
 class PathSensitiveBugReporter final : public BugReporter {
   ExprEngine& Eng;
 
   BugReport *findReportInEquivalenceClass(
       BugReportEquivClass &eqClass,
       SmallVectorImpl<BugReport *> &bugReports) override;
 
   /// Generate the diagnostics for the given bug report.
-  std::unique_ptr<DiagnosticForConsumerMapTy>
-  generateDiagnosticForConsumerMap(BugReport *exampleReport,
-                                   ArrayRef<PathDiagnosticConsumer *> consumers,
-                                   ArrayRef<BugReport *> bugReports) override;
+  std::unique_ptr<DiagnosticForConsumerMapTy> generateDiagnosticForConsumerMap(
+      BugReportEquivClass &EQ,
+      ArrayRef<PathDiagnosticConsumer *> consumers) override;
+
 public:
   PathSensitiveBugReporter(BugReporterData& d, ExprEngine& eng)
       : BugReporter(d), Eng(eng) {}
 
   /// getGraph - Get the exploded graph created by the analysis engine
   ///  for the analyzed method or function.
   const ExplodedGraph &getGraph() const;
 

clang/lib/StaticAnalyzer/Core/BugReporter.cpp

public:
   /// There are two path diagnostics generation modes: with adding edges (used
   /// for plists) and without  (used for HTML and text). When edges are added,
   /// the path is modified to insert artificially generated edges.
   /// Otherwise, more detailed diagnostics is emitted for block edges,
   /// explaining the transitions in words.
   std::unique_ptr<PathDiagnostic>
   generate(const PathDiagnosticConsumer *PDC) const;
 
+  /// Get the bug report used to generate the PathDiagnostic.
+  const PathSensitiveBugReport *getBugReport() const { return R; }
+
 private:
   void updateStackPiecesWithMessage(PathDiagnosticPieceRef P,
                                     const CallWithEntryStack &CallStack) const;
   void generatePathDiagnosticsForNode(PathDiagnosticConstruct &C,
                                       PathDiagnosticLocation &PrevLoc) const;
 
   void generateMinimalDiagForBlockEdge(PathDiagnosticConstruct &C,
                                        BlockEdge BE) const;
generateDiagForBinaryOP(const PathDiagnosticConstruct &C, const Stmt *T,
                           const CFGBlock *Src, const CFGBlock *DstC) const;
 
   PathDiagnosticLocation
   ExecutionContinues(const PathDiagnosticConstruct &C) const;
 
   PathDiagnosticLocation
   ExecutionContinues(llvm::raw_string_ostream &os,
                      const PathDiagnosticConstruct &C) const;
-
-  const PathSensitiveBugReport *getBugReport() const { return R; }
 };
 
 } // namespace
 
 //===----------------------------------------------------------------------===//
 // Base implementation of stack hint generators.
 //===----------------------------------------------------------------------===//
 
Optional<PathDiagnosticBuilder> PDB =
       PathDiagnosticBuilder::findValidReport(bugReports, *this);
 
   if (PDB) {
     for (PathDiagnosticConsumer *PC : consumers) {
       if (std::unique_ptr<PathDiagnostic> PD = PDB->generate(PC)) {
         (*Out)[PC] = std::move(PD);
       }
     }
+    Out->Report = PDB->getBugReport();
   }
 
   return Out;
 }
 
 void BugReporter::emitReport(std::unique_ptr<BugReport> R) {
   bool ValidSourceLoc = R->getLocation().isValid();
   assert(ValidSourceLoc);
while (!WL.empty()) {
         WL.pop_back();
     }
   }
 
   // ExampleReport will be NULL if all the nodes in the equivalence class
   // were post-dominated by sinks.
   return exampleReport;
 }
 
-void BugReporter::FlushReport(BugReportEquivClass& EQ) {
-  SmallVector<BugReport*, 10> bugReports;
-  BugReport *report = findReportInEquivalenceClass(EQ, bugReports);
-  if (!report)
-    return;
-
+std::unique_ptr<DiagnosticForConsumerMapTy>
+BugReporter::generateDiagnosticForConsumerMap(BugReportEquivClass &EQ) {
   // See whether we need to silence the checker/package.
+  auto *report = EQ.getReports()[0].get();
   for (const std::string &CheckerOrPackage :
        getAnalyzerOptions().SilencedCheckersAndPackages) {
     if (report->getBugType().getCheckerName().startswith(
             CheckerOrPackage))
-      return;
+      return nullptr;
   }
 
   ArrayRef<PathDiagnosticConsumer*> Consumers = getPathDiagnosticConsumers();
+  return generateDiagnosticForConsumerMap(EQ, Consumers);
+}
+
+void BugReporter::FlushReport(BugReportEquivClass &EQ) {
   std::unique_ptr<DiagnosticForConsumerMapTy> Diagnostics =
-      generateDiagnosticForConsumerMap(report, Consumers, bugReports);
+      generateDiagnosticForConsumerMap(EQ);
+  if (!Diagnostics)
+    return;
 
+  auto *report = Diagnostics->Report;
   for (auto &P : *Diagnostics) {
     PathDiagnosticConsumer *Consumer = P.first;
     std::unique_ptr<PathDiagnostic> &PD = P.second;
 
     // If the path is empty, generate a single step path with the location
     // of the issue.
     if (PD->path.empty()) {
       PathDiagnosticLocation L = report->getLocation();
while (N) {
 
     N = N->getFirstPred();
   }
   return ExecutedLines;
 }
 
 std::unique_ptr<DiagnosticForConsumerMapTy>
 BugReporter::generateDiagnosticForConsumerMap(
-    BugReport *exampleReport, ArrayRef<PathDiagnosticConsumer *> consumers,
-    ArrayRef<BugReport *> bugReports) {
-  auto *basicReport = cast<BasicBugReport>(exampleReport);
+    BugReportEquivClass &EQ, ArrayRef<PathDiagnosticConsumer *> consumers) {
+  SmallVector<BugReport *> _;
+  auto *report = BugReporter::findReportInEquivalenceClass(EQ, _);
+  if (!report)
+    return nullptr;
+
+  auto *basicReport = cast<BasicBugReport>(report);
   auto Out = std::make_unique<DiagnosticForConsumerMapTy>();
   for (auto *Consumer : consumers)
     (*Out)[Consumer] = generateDiagnosticForBasicReport(basicReport);
+  Out->Report = report;
   return Out;
 }
 
 static PathDiagnosticCallPiece *
 getFirstStackedCallToHeaderFile(PathDiagnosticCallPiece *CP,
                                 const SourceManager &SMgr) {
   SourceLocation CallLoc = CP->callEnter.asLocation();
 
if (CP) {
       PD.setDeclWithIssue(CP->getCaller());
       PD.setLocation(CP->getLocation());
 
       return;
     }
   }
 }
 
-
-
 std::unique_ptr<DiagnosticForConsumerMapTy>
 PathSensitiveBugReporter::generateDiagnosticForConsumerMap(
-    BugReport *exampleReport, ArrayRef<PathDiagnosticConsumer *> consumers,
-    ArrayRef<BugReport *> bugReports) {
+    BugReportEquivClass &EQ, ArrayRef<PathDiagnosticConsumer *> consumers) {
+  SmallVector<BugReport *, 10> bugReports;
+  const BugReport *report = findReportInEquivalenceClass(EQ, bugReports);
+  if (!report)
+    return nullptr;
+
   std::vector<BasicBugReport *> BasicBugReports;
   std::vector<PathSensitiveBugReport *> PathSensitiveBugReports;
-  if (isa<BasicBugReport>(exampleReport))
-    return BugReporter::generateDiagnosticForConsumerMap(exampleReport,
-                                                         consumers, bugReports);
+  if (isa<BasicBugReport>(report))
+    return BugReporter::generateDiagnosticForConsumerMap(EQ, consumers);
 
   // Generate the full path sensitive diagnostic, using the generation scheme
   // specified by the PathDiagnosticConsumer. Note that we have to generate
   // path diagnostics even for consumers which do not support paths, because
   // the BugReporterVisitors may mark this bug as a false positive.
   assert(!bugReports.empty());
   MaxBugClassSize.updateMax(bugReports.size());
 

clang/test/Analysis/Inputs/expected-plists/malloc-plist.c.plist

<key>edges</key>
            <key>col</key><integer>9</integer>
            <key>file</key><integer>0</integer>
           </dict>
          </array>
         <key>end</key>
          <array>
           <dict>
            <key>line</key><integer>138</integer>
-           <key>col</key><integer>9</integer>
+           <key>col</key><integer>7</integer>
            <key>file</key><integer>0</integer>
           </dict>
           <dict>
            <key>line</key><integer>138</integer>
-           <key>col</key><integer>9</integer>
+           <key>col</key><integer>7</integer>
            <key>file</key><integer>0</integer>
           </dict>
          </array>
        </dict>
       </array>
     </dict>
     <dict>
      <key>kind</key><string>event</string>
      <key>location</key>
      <dict>
       <key>line</key><integer>138</integer>
-      <key>col</key><integer>9</integer>
+      <key>col</key><integer>7</integer>
       <key>file</key><integer>0</integer>
      </dict>
      <key>depth</key><integer>1</integer>
      <key>extended_message</key>
      <string>Potential leak of memory pointed to by &apos;x&apos;</string>
      <key>message</key>
      <string>Potential leak of memory pointed to by &apos;x&apos;</string>
     </dict>
    </array>
    <key>description</key><string>Potential leak of memory pointed to by &apos;x&apos;</string>
    <key>category</key><string>Memory error</string>
    <key>type</key><string>Memory leak</string>
    <key>check_name</key><string>unix.Malloc</string>
    <!-- This hash is experimental and going to change! -->
    <key>issue_hash_content_of_line_in_context</key><string>0ddc87e8a7e7104428af3905f3057611</string>
   <key>issue_context_kind</key><string>function</string>
   <key>issue_context</key><string>function_with_leak3</string>
   <key>issue_hash_function_offset</key><string>1</string>
   <key>location</key>
   <dict>
    <key>line</key><integer>138</integer>
-   <key>col</key><integer>9</integer>
+   <key>col</key><integer>7</integer>
    <key>file</key><integer>0</integer>
   </dict>
   <key>ExecutedLines</key>
   <dict>
    <key>0</key>
    <array>
     <integer>135</integer>
     <integer>136</integer>

clang/test/Analysis/keychainAPI.m

 }
 
 void consumeChar(char);
 
 void allocNoFree2(int x) {
     OSStatus st = 0;
     void *outData = my_AllocateReturn(&st); 
     if (x) {
-      consumeChar(*(char*)outData); // expected-warning{{Allocated data is not released:}}
+      consumeChar(*(char *)outData);
       return;
     } else {
-      consumeChar(*(char*)outData);
+      consumeChar(*(char *)outData); // expected-warning{{Allocated data is not released:}}
     }
     return;
 }
 
 void allocAndFree2(void *attrList) {
     OSStatus st = 0;
     void *outData = my_AllocateReturn(&st);
     if (st == noErr)

clang/test/Analysis/malloc-plist.c

 }
 void use_function_with_leak2() {
     function_with_leak2();
 }
 
 static void function_with_leak3(int y) {
     char *x = (char*)malloc(12);
     if (y)
-        y++;
-}//expected-warning{{Potential leak}}
+      y++; // expected-warning{{Potential leak}}
+}
 void use_function_with_leak3(int y) {
     function_with_leak3(y);
 }
 
 static void function_with_leak4(int y) {
     char *x = (char*)malloc(12);
     if (y)
         y++;

clang/test/Analysis/stream.c

 
 // Check that "location uniqueing" works.
 // This results in reporting only one occurence of resource leak for a stream.
 void check_leak_noreturn_2() {
   FILE *F1 = tmpfile();
   if (!F1)
     return;
   if (Test == 1) {
-    return; // no warning
+    return; // expected-warning {{Opened stream never closed. Potential resource leak}}
   }
   rewind(F1);
-} // expected-warning {{Opened stream never closed. Potential resource leak}}
-// FIXME: This warning should be placed at the `return` above.
-// See https://reviews.llvm.org/D83120 about details.
+} // no warning