[PATCH] D15227: [analyzer] Valist checkers.

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Looks good overall; comments below.

Please, provide more information on real world code evaluation. Which codebases this has been tested on? What was the false positive rate? How many real bugs were found/fixed?

What is the criteria for taking it out of alpha?

Please, write the commit message that will be used.


================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:30
@@ +29,3 @@
+
+struct VAListAcceptingFunc {
+  mutable IdentifierInfo *II;
----------------
This looks like a general purpose utility that could be used by other checkers.
(Though, this code might not support all languages as is.)

================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:54
@@ +53,3 @@
+
+  static const SmallVector<std::pair<VAListAcceptingFunc, int>, 15> VAArgLikes;
+  static const VAListAcceptingFunc VaStart, VaEnd, VaCopy;
----------------
What are "likes" ?

================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:82
@@ +81,3 @@
+                      bool IsCopy) const;
+  void checkEndCall(const CallEvent &Call, CheckerContext &C) const;
+
----------------
Maybe these could be called "checkVAListEndCall" or something similar; as is they are too similar to "checkPreCall/checkPostCall".

================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:116
@@ +115,3 @@
+
+const SmallVector<std::pair<VAListAcceptingFunc, int>, 15>
+    ValistChecker::VAArgLikes = {{{"vfprintf", 3}, 2},
----------------
Explain what's in the pair or make it into a struct. (I suspect the second argument is the index of the argument taking the va_arg.)

================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:204
@@ +203,3 @@
+
+StringRef ValistChecker::getVariableNameFromRegion(const MemRegion *Reg) const {
+  const ElementRegion *EReg;
----------------
This should be a general-purpose utility. It's not a good idea for each checker to roll their own implementation:) See 'variableName' in http://reviews.llvm.org/D12761.

There could be other places in the analyzer where we print the names. 


================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:217
@@ +216,3 @@
+
+const ExplodedNode *ValistChecker::getStartCallSite(const ExplodedNode *N,
+                                                    const MemRegion *Reg,
----------------
Please, add a comment explaining what this does and roughly how.

Looks like the main difference with Malloc checker's getAllocationSite is that this does not look for the ReferenceRegion. That might not matter for va lists since the usage is not likely to span many inlined functions, is that correct?

================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:361
@@ +360,3 @@
+
+PathDiagnosticPiece *ValistChecker::ValistBugVisitor::VisitNode(
+    const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
----------------
Please add tests for this, preferably the plist output.

================
Comment at: test/Analysis/Inputs/system-header-simulator-for-valist.h:4
@@ +3,3 @@
+// 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.
----------------
Does the second part of the comment refer to something you test for here?

================
Comment at: test/Analysis/Inputs/system-header-simulator-valist.h:3
@@ +2,3 @@
+
+#pragma clang system_header
+
----------------
Is this an unused duplicate file?

================
Comment at: test/Analysis/valist-uninitialized.c:64
@@ +63,3 @@
+
+//this only contains problems which are handled by varargs.Unterminated
+void reinit(int *fst, ...) {
----------------
http://llvm.org/docs/CodingStandards.html#commenting

================
Comment at: test/Analysis/valist-uninitialized.c:98
@@ +97,3 @@
+  va_end(va);
+} // no-warning
+
----------------
The majority of functions end with "//no-warning" comment. Why is that?

================
Comment at: test/Analysis/valist-uninitialized.c:134
@@ +133,3 @@
+
+// NOTE: this is invalid, as the man page of va_end requires that "Each invocation of va_start()
+// must be matched by a corresponding  invocation of va_end() in the same function."
----------------
Is this something we would want to warn on or not?

================
Comment at: test/Analysis/valist-uninitialized.c:146
@@ +145,3 @@
+// and the warning is generated during the analysis of call_uses_arg2()
+void uses_arg2(va_list arg) {
+  (void)va_arg(arg, int); //expected-warning{{va_arg() is called on an uninitialized va_list}}
----------------
You could call this "inlined_uses_arg".


http://reviews.llvm.org/D15227