[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:836-837
+          NewState, NewNode,
+          C.getNoteTag([Msg](PathSensitiveBugReport &BR,
+                             llvm::raw_ostream &OS) { OS << Msg; }));
     }
----------------
This way each and every applied constraint will be displayed even if the given argument does not constitute to the bug condition.
I recommend you branching within the lambda, on the interestingness of the given argument constraint.


================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:247-254
   __two_constrained_args(x, y);
+  // bugpath-note at -1{{Applied constraint: The 1st arg should be within the range [1, 1]}}
+  // bugpath-note at -2{{Applied constraint: The 2nd arg should be within the range [1, 1]}}
+  //NOTE! Because of the second `clang_analyzer_eval` call we have two bug
+  //reports, thus the 'Applied constraint' notes appear twice.
+  // bugpath-note at -5{{Applied constraint: The 1st arg should be within the range [1, 1]}}
+  // bugpath-note at -6{{Applied constraint: The 2nd arg should be within the range [1, 1]}}
----------------
nit.
BTW I raised my related concerns about this elsewhere.


================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:264-276
 int __arg_constrained_twice(int);
 void test_multiple_constraints_on_same_arg(int x) {
   __arg_constrained_twice(x);
+  // bugpath-note at -1{{The 1st arg should be out of the range [1, 1]}}
+  // bugpath-note at -2{{The 1st arg should be out of the range [2, 2]}}
   // Check that both constraints are applied and only one branch is there.
   clang_analyzer_eval(x < 1 || x > 2); // \
----------------
I was puzzled for a moment on why do you have two notes here.
By checking the definition of `__arg_constrained_twice()`, I can see that it has **two** `ArgConstraint`s.

Although, it shouldn't be a problem as on the UI we would visualize notes for only a single bugreport. So it is probably clear that both of the notes are valid and correspond to the given statement. It might look clunky in the LIT test, but should be 'somewhat' readable in real life.

You should take no action here. I leave this comment just for the record.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101526