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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits