[PATCH] D122285: [analyzer] Add path note tags to standard library function summaries.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:872-883
+    if (NewState && NewState != State) {
+      StringRef Note = Case.getNote();
+      const NoteTag *Tag = C.getNoteTag(
+          // Sorry couldn't help myself.
+          [Node, Note]() {
+            // Don't emit "Assuming..." note when we ended up
+            // knowing in advance which branch is taken.
----------------
steakhal wrote:
> I thought we mostly have one or two cases. If we have two cases, then the constraint should be the opposite of the other one.
> 
> If this is the case, when does `NewState && NewState != State` not imply `Node->succ_size() > 1` given that `Summary.getCases().size() >= 2`?
Typically this is going to be the case but I wouldn't rely on that. The constraint solver may get confused (inb4 "system is over constrained"), there may be other updates to the state in the branch definition which would cause the state to change.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:879
+            // knowing in advance which branch is taken.
+            return (Node->succ_size() > 1) ? Note.str() : "";
+          },
----------------
balazske wrote:
> balazske wrote:
> > Can other checkers not add successor nodes (that make this check not always correct)?
> Is there a reason against add the note without the word "Assuming" instead of no note?
> Can other checkers not add successor nodes (that make this check not always correct)?

They'll be chained to the newly created node, not to the same predecessor. Two checkers adding transitions on the same post-call won't (and shouldn't) suddenly cause a state split.

> Is there a reason against add the note without the word "Assuming" instead of no note?

We generally never have event notes in such cases.

We sometimes have control flow notes such as "`aking true branch`" (arrow pointing to the branch in plist).

Then, separately from that, we have tracking notes such as "an interesting value appeared in this variable from this assignment, which also makes the assigned value interesting" (eg. "`null pointer value assigned to 'p'`") - we could have a similar note "an interesting value was returned from that function because that other value had a certain range, which also makes that other value interesting" - which we should definitely consider.

But simply saying "this function call was completely predictable and we don't even care what the value is in the first place" doesn't sound useful.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1326-1330
           // The locale-specific range.
           .Case({ArgumentCondition(0U, WithinRange, {{128, UCharRangeMax}})})
           // Is not an unsigned char.
           .Case({ArgumentCondition(0U, OutOfRange, Range(0, UCharRangeMax)),
                  ReturnValueCondition(WithinRange, SingleValue(0))}));
----------------
steakhal wrote:
> Why don't we have notes for these cases?
Like I mentioned in D122285#inline-1169194, I'm not sure these cases should exist at all. In particular, explaining them to the user is fairly problematic because justifying them in the first place is problematic.


================
Comment at: clang/test/Analysis/std-c-library-functions-path-notes.c:3
+// RUN:     -analyzer-checker=core,apiModeling \
+// RUN:     -analyzer-config assume-controlled-environment=false \
+// RUN:     -analyzer-output=text
----------------
steakhal wrote:
> This is the default value. What's the benefit of passing this?
I'm not sure what the default value //should// be. I guess I can remove this and/or move non-`getenv` tests into another file.


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

https://reviews.llvm.org/D122285