[PATCH] D15227: [analyzer] Valist checkers.

[Artem Dergachev via cfe-commits]

NoQ added a comment.

The checker's in great shape! I see a few minor things, but that's it.

The checks are split into two sections ("uninitialized" and "unterminated"), but there seem to be more auxiliary checks provided (eg. "copies into itself") that are on for both checkers, do you think you need to add more flags to make the checker more flexible, or maybe just merge everything into a single checker?

Because plist tests are so heavy: if all you wanted to test was how your bug reporter visitor works, did you consider using `-analyzer-output=text` instead? (this is not the default output mode; it has `note:` diagnostics displaying the path, which you can catch with `expected-note{{}}`). Also, you could probably write special tests for the visitor, instead of testing the visitors on all tests, regardless of text/plist. But i'm merely sharing this hint, i don't think it's worth rewriting once we already have plist tests.


================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:177
@@ +176,3 @@
+  if (ExplodedNode *N = C.addTransition(State))
+    reportLeakedVALists(LeakedVALists, "Initialized va_list", " is leaked", C,
+                        N);
----------------
dcoughlin wrote:
> Are there tests for this diagnostic? I don't see any.
In the other file, first few tests in `valist-unterminated.c`.

================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:185
@@ +184,3 @@
+  const auto *TReg = dyn_cast_or_null<TypedValueRegion>(Reg);
+  const auto *EReg = dyn_cast_or_null<ElementRegion>(TReg);
+  return EReg ? EReg->getSuperRegion() : TReg;
----------------
At first, i thought that you're trying to strip casts from symbolic pointers here. But then i figured out that you are stripping an implementation detail here - as even `VarRegion`-based VAs reach here in the form of "`element{va,0 S64b,struct __va_list_tag}`". I've a feeling this deserves a comment.

================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:254
@@ +253,3 @@
+    const Stmt *StartCallStmt = nullptr;
+    if (Optional<CallExitEnd> Exit = P.getAs<CallExitEnd>())
+      StartCallStmt = Exit->getCalleeContext()->getCallSite();
----------------
Does `static const Stmt *PathDiagnosticLocation::getStmt(const ExplodedNode *)` work for you? Because i guess it was specifically designed for that purpose. Here and in one more place where this code is duplicated.

================
Comment at: test/Analysis/valist-uninitialized.c:64
@@ +63,3 @@
+  (void)va_arg(*y, int); //expected-warning{{va_arg() is called on an uninitialized va_list}}
+} // no-warning 
+
----------------
Trailing whitespace :)

================
Comment at: test/Analysis/valist-uninitialized.c:98
@@ +97,3 @@
+  va_start(va, fst);
+  va_copy(va, va); // expected-warning{{va_list 'va' is copied onto itself}} 
+  va_end(va);
----------------
A bit more trailing whitespace here and below.

================
Comment at: test/Analysis/valist-uninitialized.c:178
@@ +177,3 @@
+  va_list va;
+  some_library_function(n, va);
+} //no-warning
----------------
> Like the compiler, the static analyzer treats some functions differently if
> they come from a system header -- for example, //it is assumed that system//
> functions do not arbitrarily free() their parameters//, and that some bugs
> found in system headers cannot be fixed by the user and should be
> suppressed.

Perhaps library functions can be assumed to never `va_end()` `va_list`s either. The idea behind the trick, which is also used in, say, `SimpleStreamChecker`, is that we know all library functions, we can list all library functions that `va_end()` `va_list`s, so it's safe to assume that all other library functions do not `va_end()` `va_list`s (completely unlike other, non-standard functions with unavailable bodies).

So i think this test should warn, and it shouldn't be hard to fix. That said, some checkers (eg. `PthreadLockChecker`) do not implement this trick, so it's not critical, but it may give you slightly more positives.

================
Comment at: test/Analysis/valist-unterminated.c:42
@@ +41,3 @@
+}
+  //FIXME: this should NOT cause a warning
+
----------------
Wow, these tests are mind-breaking :))

Is it ok if i ask to put the FIXME above the test or above no-warning or into function name? Because it took some time for me to figure out that `f7` is not FIXME, being used to the traditional positioning of the FIXME comment><


https://reviews.llvm.org/D15227