[PATCH] D145069: [analyzer][NFC] Split the no state change logic and bug report suppression into two visitors

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

Szelethus added a comment.

We worked on this together, so I waited a bit for others to have a say in this, but this design seems like a no brainer to me. Please fix those comments, otherwise LGTM.

Also, while the patch is LGTM (moving code around is okay), the comment says "system headers having a chance to initialize the value but failing to do so", but do we ever check anywhere whether the function actually had the chance to change the value (passed by reference/pointer)?



================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:507-515
   // Optimistically suppress uninitialized value bugs that result
   // from system headers having a chance to initialize the value
   // but failing to do so. It's too unlikely a system header's fault.
   // It's much more likely a situation in which the function has a failure
   // mode that the user decided not to check. If we want to hunt such
   // omitted checks, we should provide an explicit function-specific note
   // describing the precondition under which the function isn't supposed to
----------------
Since we don't check within this visitor whether the value is uninitialized, these comments are no longer up-to-date (and were not before your patch either).

What we are really doing here is flat out suppressing the report if it contains an inlined standard library function. What also needs to be said is that this visitor is as dumb as it comes -- its really down to the caller (or, more specifically, the one who registers this visitor) to make sure that this heuristic should be employed.

For example, if the bug report is about an uninitialized value that was passed to a system header function as a pointer/reference, this visitor could suppress the warning on the grounds that the system header likely would have initialized this value, but in some obscure cases the function could fail and not initialize the value, which could theoretically occur but practice it yields only (or mostly) false positives.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:517-523
     // We make an exception for system header functions that have no branches.
     // Such functions unconditionally fail to initialize the variable.
     // If they call other functions that have more paths within them,
     // this suppression would still apply when we visit these inner functions.
     // One common example of a standard function that doesn't ever initialize
     // its out parameter is operator placement new; it's up to the follow-up
     // constructor (if any) to initialize the memory.
----------------
Here as well, drop the bit about uninitializedness, unless you talk about it in the context of an example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145069