[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I took a look at the results. You can take a look at selected ones here:

http://cc.elte.hu:15001/Default/#report-hash=&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&is-unique=on

Due to the lack of better categorization, I used the following (so this has in fact no relation to whether the report is correct or not):

- Intentional reports (green x) are the ones where the bugreport got unquestionably worse.
- Confirmed reports (red check mark) got definitely better
- False positives (grey line) are in between.

If you hover your mouse over the icon, you can also read a comment of mine. When you open a report, in the right corner of the source code you'll find a dropdown menu, allowing you to see the report without this patch applied:
F9217110: image.png <https://reviews.llvm.org/F9217110>
There, you can observe the original bug report without this patch.

Some conclusions:

- Cases where the condition is also a variable initialization tend to expand the bug path greatly. This isn't always bad, but should be noted. In general, unless we have a good heuristic to figure out whether there is meaningful information on the right hand side of the initialization, I think we just shouldn't track these. A good example for this is this one <http://cc.elte.hu:15001/Default/#is-unique=on&report-hash=&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&reportHash=20eb450bc76b250d0ffa40e0435d3823&report=2998&subtab=20eb450bc76b250d0ffa40e0435d3823>. The 37th event contains pretty much every information we need, it's obvious that the optional could either be None or non-None, since it's in a condition. dyn_cast is a prime example too: in this case <http://cc.elte.hu:15001/Default/#is-unique=on&report-hash=&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&reportHash=ae426512e10d1835e2b80b594d41226a&report=3126&subtab=ae426512e10d1835e2b80b594d41226a>, note 9 is all we really need.
- We should probably believe that `operator bool()` is implemented correctly, and shouldn't track the value all the way there (at least, when we're only tracking the condition). Example <http://cc.elte.hu:15001/Default/#is-unique=on&report-hash=&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&reportHash=cb40d620796d9fb5257657817766ef6c&report=3775&subtab=cb40d620796d9fb5257657817766ef6c>
- We shouldn't ever track assert-like conditions. Example <http://cc.elte.hu:15001/Default/#is-unique=on&report-hash=&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&reportHash=4a863a6cf99be5676bc7d1d9aedccfb1&report=3087&subtab=4a863a6cf99be5676bc7d1d9aedccfb1> (note 38-41)
- When the report didn't suffer from any of the above issues, I found the extra notes to be helpful! :D

I'm doing another analysis to help a bit on evaluation with the following modification:

  diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  index 9c40ebead56..4ac4b873675 100644
  --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  @@ -1892,12 +1892,28 @@ TrackControlDependencyCondBRVisitor::VisitNode(const ExplodedNode *N,
     if (!OriginB || !NB)
       return nullptr;
   
  -  if (ControlDepTree.isControlDependency(OriginB, NB))
  -    if (const Expr *Condition = getTerminatorCondition(NB))
  -      if (BR.addTrackedCondition(Condition))
  +  if (ControlDepTree.isControlDependency(OriginB, NB)) {
  +    if (const Expr *Condition = getTerminatorCondition(NB)) {
  +      if (BR.addTrackedCondition(Condition)) {
           bugreporter::trackExpressionValue(
               N, Condition, BR, /*EnableNullFPSuppression=*/false);
   
  +        if (BRC.getAnalyzerOptions().AnalysisDiagOpt == PD_NONE)
  +          return nullptr;
  +
  +        std::string ConditionText = Lexer::getSourceText(
  +            CharSourceRange::getTokenRange(Condition->getSourceRange()),
  +                                           BRC.getSourceManager(),
  +                                           BRC.getASTContext().getLangOpts());
  +
  +        return std::make_shared<PathDiagnosticEventPiece>(
  +            PathDiagnosticLocation::createEnd(
  +                Condition, BRC.getSourceManager(), N->getLocationContext()),
  +                "Tracking condition " + ConditionText);
  +      }
  +    }
  +  }
  +
     return nullptr;
   }


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

https://reviews.llvm.org/D62883