[PATCH] D83961: [Analyzer] Fix bug report source locations in minimal output.

Mon Jul 20 06:04:30 PDT 2020

balazske marked an inline comment as done.
balazske added a comment.

The problem in D83120 <https://reviews.llvm.org/D83120> is fixed by this patch.
What I figured out from the code is that `BugReporter::FlushReport` calls `findReportInEquivalenceClass` that returns a report that has not necessary the shortest path. That report is get from the passed bug report class and a different instance is returned if the uniqueing is turned on or off. This report is used to fill the last location in the bug path, if the bug path is empty. So the location in the bug path changes dependent on the setting of uniqueing. The bug path here is empty if the analyzer-output is set to minimal (or not specified). Even in the minimal output case, function `PathDiagnosticBuilder::generate` is called if it is a path sensitive case. In that function the shortest path is used. So instead of returning an empty path from that function (what is filled later by FlushReport) the last part of the path can be filled here (and use the correct source location from the shortest path).
The problem can happen if not the report with shortest path is returned from `findReportInEquivalenceClass`, and analyzer-output is set to minimal or default. If the output is not minimal, the bug path is filled by the `PathDiagnosticBuilder::generate` with correct locations. So changing the output type can result in change of the warning location.
Other way to get the problem is change only the uniqueing setting in bug type and use minimal (or default) output. Then the example report can change and because this example report is used to get the bug location the location can change too (if the equivalence class contains paths with different end locations).

================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:3007
     // of the issue.
+    // This can happen if report is BasicBugReport.
     if (PD->path.empty()) {
----------------
Szelethus wrote:
> Is this the *only* instance when that happens? How about the `text-minimal` output?
I am not totally sure on this but inserted an assert and all the tests passed without triggering it.
Is the text-minimal different from the default (when to `--analyzer-output` is given at all)?
I think it works like this: Function `PathSensitiveBugReporter::generateDiagnosticForConsumerMap` is called from here, then if it is not a `BasicBugReport` the function `PathDiagnosticBuilder::generate` will be called that is fixed now to return always with non-empty path.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83961/new/

https://reviews.llvm.org/D83961