[PATCH] D15227: [analyzer] Valist checkers.

[Gábor Horváth via cfe-commits]

xazax.hun marked 9 inline comments as done.
xazax.hun added a comment.

It was tested on gcc and rAthena (https://github.com/rathena/rathena). It did not find any issues in those projects, but I was able to find some issues that I artificially put into those projects.


================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:30
@@ +29,3 @@
+
+struct VAListAcceptingFunc {
+  mutable IdentifierInfo *II;
----------------
zaks.anna wrote:
> This looks like a general purpose utility that could be used by other checkers.
> (Though, this code might not support all languages as is.)
I agree.
Right now it does not support namespaces, overloading on the types of the argument, overloading on CV qualifiers etc. Do you have any other language features that is not supported in mind?
Supporting all of that might be useful but I am a bit afraid of the result being bloated. What do you think, what would be the most appropriate place to put such utility? It could be a helper class in CallEvent.cpp/h.

================
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;
----------------
zaks.anna wrote:
> What are "likes" ?
va_list accepting functions.

================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:82
@@ +81,3 @@
+                      bool IsCopy) const;
+  void checkEndCall(const CallEvent &Call, CheckerContext &C) const;
+
----------------
zaks.anna wrote:
> Maybe these could be called "checkVAListEndCall" or something similar; as is they are too similar to "checkPreCall/checkPostCall".
You are right, I will rename this to better reflect their role.

================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:204
@@ +203,3 @@
+
+StringRef ValistChecker::getVariableNameFromRegion(const MemRegion *Reg) const {
+  const ElementRegion *EReg;
----------------
zaks.anna wrote:
> 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. 
> 
I agree that it would be great to have this as a general utility. I think I should wait until the implementation from the other checker gets commited.

================
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:217
@@ +216,3 @@
+
+const ExplodedNode *ValistChecker::getStartCallSite(const ExplodedNode *N,
+                                                    const MemRegion *Reg,
----------------
zaks.anna wrote:
> 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?
Right. Clarified this in the comment.

================
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.
----------------
zaks.anna wrote:
> Does the second part of the comment refer to something you test for here?
Added a test for it.

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

================
Comment at: test/Analysis/valist-uninitialized.c:98
@@ +97,3 @@
+  va_end(va);
+} // no-warning
+
----------------
zaks.anna wrote:
> The majority of functions end with "//no-warning" comment. Why is that?
To indicate that, no leak related warning there. Do you prefer to remove those comments?

================
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."
----------------
zaks.anna wrote:
> Is this something we would want to warn on or not?
We should definitely wanr on that, but I think it should be ok to add this feature later.


http://reviews.llvm.org/D15227