[PATCH] D15227: [analyzer] Valist checkers.
xazax.hun added a comment. In https://reviews.llvm.org/D15227#681127, @zaks.anna wrote: > > But as far as I remember, this produced false negatives in the tests not > > false positives. > > Could you double check that? Maybe you still have some notes in your mail box > or just by looking at the code. > > Did none of the checks work or just some of them? > > Also, which platforms did this not work on? > > It would be great to move all of the useful checks out of alpha since alpha > essentially means "unsupported" and we do not recommend turning those > checkers on. > > Thanks! I think I managed to solve the problems due to the different AST variants. In the future this should be abstracted away by the analyzer core. You can see the patch here: https://reviews.llvm.org/D30157 Early next week I will rerun this check on a larger open source project. If it does not give me too much false positives or crash, I think it is a good candidate to move out from alpha. Repository: rL LLVM https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15227: [analyzer] Valist checkers.
zaks.anna added a comment. > But as far as I remember, this produced false negatives in the tests not > false positives. Could you double check that? Maybe you still have some notes in your mail box or just by looking at the code. Did none of the checks work or just some of them? Also, which platforms did this not work on? It would be great to move all of the useful checks out of alpha since alpha essentially means "unsupported" and we do not recommend turning those checkers on. Thanks! Repository: rL LLVM https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15227: [analyzer] Valist checkers.
xazax.hun added a comment. In https://reviews.llvm.org/D15227#674278, @zaks.anna wrote: > @xazax.hun, > > Can we move this out of alpha? > > Have this checkers been tested on a large codebase? What are false positive > rates? I have tested it on a few ~200k LOC C codebase and I did not see any major problem. There is one problem left though, when I committed this check some of the build bots broke, I suspect that slightly different AST is generated on some of the platforms. I temporarily fixed the issue by hardcoding the triple into the tests and I did not have time yet to investigate the source of the problems. But as far as I remember, this produced false negatives in the tests not false positives. Repository: rL LLVM https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15227: [analyzer] Valist checkers.
zaks.anna added a comment. @xazax.hun, Can we move this out of alpha? Have this checkers been tested on a large codebase? What are false positive rates? Thanks! Anna Repository: rL LLVM https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15227: [analyzer] Valist checkers.
xazax.hun added a comment. In https://reviews.llvm.org/D15227#521781, @NoQ wrote: > In https://reviews.llvm.org/D15227#519239, @xazax.hun wrote: > > > I suspect that slightly different AST is generated for those architectures > > that cause the different behavior. I will further investigate those > > problems. > > > Seems so, because on my machine when i append `-triple hexagon-unknown-linux` > to the test run line, a bunch of things start failing. You may also consider > hardcoding the runline in the current test and commit (which is so often a > good idea), and fix other stuff in later commits. A proper investigation > would be great, of course :) I committed with the hardcoded triple in the runline for now. > P.S. Just curious - do you plan to eventually model `va_arg()`, as we > discussed with Michael Tandy in > http://lists.llvm.org/pipermail/cfe-dev/2016-August/050202.html ? It is not on my TODO list at the moment, but I might look into it in the distant future. Repository: rL LLVM https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15227: [analyzer] Valist checkers.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL279427: Reapply "[analyzer] Added valist related checkers."
(authored by xazax).
Repository:
rL LLVM
https://reviews.llvm.org/D15227
Files:
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
cfe/trunk/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-valist.h
cfe/trunk/test/Analysis/valist-uninitialized.c
cfe/trunk/test/Analysis/valist-unterminated.c
Index: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -43,6 +43,9 @@
def Cplusplus : Package<"cplusplus">;
def CplusplusAlpha : Package<"cplusplus">, InPackage, Hidden;
+def Valist : Package<"valist">;
+def ValistAlpha : Package<"valist">, InPackage, Hidden;
+
def DeadCode : Package<"deadcode">;
def DeadCodeAlpha : Package<"deadcode">, InPackage, Hidden;
@@ -267,6 +270,27 @@
} // end: "alpha.cplusplus"
+
+//===--===//
+// Valist checkers.
+//===--===//
+
+let ParentPackage = ValistAlpha in {
+
+def UninitializedChecker : Checker<"Uninitialized">,
+ HelpText<"Check for usages of uninitialized (or already released) va_lists.">,
+ DescFile<"ValistChecker.cpp">;
+
+def UnterminatedChecker : Checker<"Unterminated">,
+ HelpText<"Check for va_lists which are not released by a va_end call.">,
+ DescFile<"ValistChecker.cpp">;
+
+def CopyToSelfChecker : Checker<"CopyToSelf">,
+ HelpText<"Check for va_lists which are copied onto itself.">,
+ DescFile<"ValistChecker.cpp">;
+
+} // end : "alpha.valist"
+
//===--===//
// Deadcode checkers.
//===--===//
Index: cfe/trunk/test/Analysis/valist-unterminated.c
===
--- cfe/trunk/test/Analysis/valist-unterminated.c
+++ cfe/trunk/test/Analysis/valist-unterminated.c
@@ -0,0 +1,133 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -analyze -analyzer-checker=core,alpha.valist.Unterminated,alpha.valist.CopyToSelf -analyzer-output=text -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator-for-valist.h"
+
+void f1(int fst, ...) {
+ va_list va;
+ va_start(va, fst); // expected-note{{Initialized va_list}}
+ return; // expected-warning{{Initialized va_list 'va' is leaked}} expected-note{{Initialized va_list 'va' is leaked}}
+}
+
+void f2(int fst, ...) {
+ va_list va;
+ va_start(va, fst); // expected-note{{Initialized va_list}}
+ va_end(va); // expected-note{{Ended va_list}}
+ va_start(va, fst); // expected-note{{Initialized va_list}}
+} // expected-warning{{Initialized va_list 'va' is leaked}} expected-note{{Initialized va_list 'va' is leaked}}}
+
+void f3(int fst, ...) {
+ va_list va, va2;
+ va_start(va, fst);
+ va_copy(va2, va); // expected-note{{Initialized va_list}}
+ va_end(va); // expected-warning{{Initialized va_list 'va2' is leaked}} expected-note{{Initialized va_list 'va2' is leaked}}
+}
+
+void f4(va_list *fst, ...) {
+ va_start(*fst, fst); // expected-note{{Initialized va_list}}
+ return; // expected-warning{{Initialized va_list is leaked}} expected-note{{Initialized va_list is leaked}}
+}
+
+void f5(va_list fst, ...) {
+ va_start(fst, fst);
+ //FIXME: this should cause a warning
+} // no-warning
+
+void f6(va_list *fst, ...) {
+ va_start(*fst, fst); // expected-note{{Initialized va_list}}
+ (void)va_arg(*fst, int);
+ //FIXME: this should NOT cause a warning
+ va_end(*fst); // expected-warning{{Initialized va_list is leaked}} expected-note{{Initialized va_list is leaked}}
+}
+
+void f7(int *fst, ...) {
+ va_list x;
+ va_list *y =
+ va_start(*y,fst); // expected-note{{Initialized va_list}}
+} // expected-warning{{Initialized va_list 'x' is leaked}} expected-note{{Initialized va_list 'x' is leaked}}
+
+void f8(int *fst, ...) {
+ va_list x;
+ va_list *y =
+ va_start(*y,fst);
+ va_end(x);
+} // no-warning
+
+void reinit(int *fst, ...) {
+ va_list va;
+ va_start(va, fst); // expected-note{{Initialized va_list}} expected-note{{Initialized va_list}}
+ va_start(va, fst); // expected-warning{{Initialized va_list 'va' is initialized again}} expected-note{{Initialized va_list 'va' is initialized again}}
+} // expected-warning{{Initialized va_list 'va' is leaked}} expected-note{{Initialized va_list 'va' is leaked}}
+
+void reinitOk(int *fst, ...) {
+ va_list va;
+ va_start(va, fst);
+ va_end(va);
+ va_start(va, fst);
+ va_end(va);
+} // no-warning
+
+void copyself(int fst, ...) {
+ va_list va;
+
Re: [PATCH] D15227: [analyzer] Valist checkers.
NoQ added a comment. In https://reviews.llvm.org/D15227#519239, @xazax.hun wrote: > I suspect that slightly different AST is generated for those architectures > that cause the different behavior. I will further investigate those problems. Seems so, because on my machine when i append `-triple hexagon-unknown-linux` to the test run line, a bunch of things start failing. You may also consider hardcoding the runline in the current test and commit (which is so often a good idea), and fix other stuff in later commits. A proper investigation would be great, of course :) P.S. Just curious - do you plan to eventually model `va_arg()`, as we discussed with Michael Tandy in http://lists.llvm.org/pipermail/cfe-dev/2016-August/050202.html ? Repository: rL LLVM https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15227: [analyzer] Valist checkers.
xazax.hun reopened this revision. xazax.hun added a comment. This revision is now accepted and ready to land. It looks like it broke some of the build bots. Error from the windows build bots: error: 'note' diagnostics expected but not seen: File C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\Analysis\valist-uninitialized.c Line 96: va_list 'va' is copied onto itself File C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\Analysis\valist-uninitialized.c Line 102: va_list 'va' is copied onto itself File C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\Analysis\valist-uninitialized.c Line 108: Initialized va_list 'va' is overwritten by an uninitialized one error: 'note' diagnostics seen but not expected: Line 96: va_list va' is copied onto itself Line 102: va_list va' is copied onto itself Line 108: Initialized va_list va' is overwritten by an uninitialized one 12 errors generated. Somehow the beginning single quote is missing from the generated message. It is very strange. Error from other architectures: error: 'warning' diagnostics expected but not seen: File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 27: Initialized va_list is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 39: Initialized va_list is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array[3]' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem[0]' is leaked error: 'warning' diagnostics seen but not expected: File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 33: Initialized va_list 'fst' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem' is leaked error: 'note' diagnostics expected but not seen: File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 26: Initialized va_list File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 27: Initialized va_list is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 36: Initialized va_list File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 39: Initialized va_list is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array[3]' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem[0]' is leaked error: 'note' diagnostics seen but not expected: Line 31: Initialized va_list File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 33: Initialized va_list 'fst' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array' is leaked File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem' is leaked 17 errors generated. I suspect that slightly different AST is generated for those architectures that cause the different behavior. I will further investigate those problems. Repository: rL LLVM https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15227: [analyzer] Valist checkers.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL279041: [analyzer] Added valist related checkers. (authored
by xazax).
Changed prior to commit:
https://reviews.llvm.org/D15227?vs=68157=68497#toc
Repository:
rL LLVM
https://reviews.llvm.org/D15227
Files:
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
cfe/trunk/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-valist.h
cfe/trunk/test/Analysis/valist-uninitialized.c
cfe/trunk/test/Analysis/valist-unterminated.c
Index: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -43,6 +43,9 @@
def Cplusplus : Package<"cplusplus">;
def CplusplusAlpha : Package<"cplusplus">, InPackage, Hidden;
+def Valist : Package<"valist">;
+def ValistAlpha : Package<"valist">, InPackage, Hidden;
+
def DeadCode : Package<"deadcode">;
def DeadCodeAlpha : Package<"deadcode">, InPackage, Hidden;
@@ -267,6 +270,27 @@
} // end: "alpha.cplusplus"
+
+//===--===//
+// Valist checkers.
+//===--===//
+
+let ParentPackage = ValistAlpha in {
+
+def UninitializedChecker : Checker<"Uninitialized">,
+ HelpText<"Check for usages of uninitialized (or already released) va_lists.">,
+ DescFile<"ValistChecker.cpp">;
+
+def UnterminatedChecker : Checker<"Unterminated">,
+ HelpText<"Check for va_lists which are not released by a va_end call.">,
+ DescFile<"ValistChecker.cpp">;
+
+def CopyToSelfChecker : Checker<"CopyToSelf">,
+ HelpText<"Check for va_lists which are copied onto itself.">,
+ DescFile<"ValistChecker.cpp">;
+
+} // end : "alpha.valist"
+
//===--===//
// Deadcode checkers.
//===--===//
Index: cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-valist.h
===
--- cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-valist.h
+++ cfe/trunk/test/Analysis/Inputs/system-header-simulator-for-valist.h
@@ -0,0 +1,30 @@
+// 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.
+
+#pragma clang system_header
+
+#ifdef __cplusplus
+#define restrict /*restrict*/
+#endif
+
+typedef __builtin_va_list va_list;
+
+#define va_start(ap, param) __builtin_va_start(ap, param)
+#define va_end(ap) __builtin_va_end(ap)
+#define va_arg(ap, type)__builtin_va_arg(ap, type)
+#define va_copy(dst, src) __builtin_va_copy(dst, src)
+
+int vprintf (const char *restrict format, va_list arg);
+
+int vsprintf (char *restrict s, const char *restrict format, va_list arg);
+
+int some_library_function(int n, va_list arg);
+
+// No warning from system header.
+inline void __impl_detail(int fst, ...) {
+ va_list va;
+ (void)va_arg(va, int);
+}
Index: cfe/trunk/test/Analysis/valist-uninitialized.c
===
--- cfe/trunk/test/Analysis/valist-uninitialized.c
+++ cfe/trunk/test/Analysis/valist-uninitialized.c
@@ -0,0 +1,178 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.valist.Uninitialized,alpha.valist.CopyToSelf -analyzer-output=text -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator-for-valist.h"
+
+void f1(int fst, ...) {
+ va_list va;
+ (void)va_arg(va, int); //expected-warning{{va_arg() is called on an uninitialized va_list}} expected-note{{va_arg() is called on an uninitialized va_list}}
+}
+
+int f2(int fst, ...) {
+ va_list va;
+ va_start(va, fst); // expected-note{{Initialized va_list}}
+ va_end(va); // expected-note{{Ended va_list}}
+ return va_arg(va, int); //expected-warning{{va_arg() is called on an uninitialized va_list}} expected-note{{va_arg() is called on an uninitialized va_list}}
+}
+
+void f3(int fst, ...) {
+ va_list va, va2;
+ va_start(va, fst);
+ va_copy(va2, va);
+ va_end(va);
+ (void)va_arg(va2, int);
+ va_end(va2);
+} //no-warning
+
+void f4(int cond, ...) {
+ va_list va;
+ if (cond) { // expected-note{{Assuming 'cond' is 0}} expected-note{{Taking false branch}}
+va_start(va, cond);
+(void)va_arg(va,int);
+ }
+ va_end(va); //expected-warning{{va_end() is called on an uninitialized va_list}} expected-note{{va_end() is called on an uninitialized va_list}}
+}
+
+void
Re: [PATCH] D15227: [analyzer] Valist checkers.
xazax.hun updated this revision to Diff 68157.
xazax.hun marked 5 inline comments as done.
xazax.hun added a comment.
- Improvements according to review comments.
https://reviews.llvm.org/D15227
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/ValistChecker.cpp
test/Analysis/Inputs/system-header-simulator-for-valist.h
test/Analysis/valist-uninitialized.c
test/Analysis/valist-unterminated.c
Index: test/Analysis/valist-unterminated.c
===
--- /dev/null
+++ test/Analysis/valist-unterminated.c
@@ -0,0 +1,133 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.valist.Unterminated,alpha.valist.CopyToSelf -analyzer-output=text -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator-for-valist.h"
+
+void f1(int fst, ...) {
+ va_list va;
+ va_start(va, fst); // expected-note{{Initialized va_list}}
+ return; // expected-warning{{Initialized va_list 'va' is leaked}} expected-note{{Initialized va_list 'va' is leaked}}
+}
+
+void f2(int fst, ...) {
+ va_list va;
+ va_start(va, fst); // expected-note{{Initialized va_list}}
+ va_end(va); // expected-note{{Ended va_list}}
+ va_start(va, fst); // expected-note{{Initialized va_list}}
+} // expected-warning{{Initialized va_list 'va' is leaked}} expected-note{{Initialized va_list 'va' is leaked}}}
+
+void f3(int fst, ...) {
+ va_list va, va2;
+ va_start(va, fst);
+ va_copy(va2, va); // expected-note{{Initialized va_list}}
+ va_end(va); // expected-warning{{Initialized va_list 'va2' is leaked}} expected-note{{Initialized va_list 'va2' is leaked}}
+}
+
+void f4(va_list *fst, ...) {
+ va_start(*fst, fst); // expected-note{{Initialized va_list}}
+ return; // expected-warning{{Initialized va_list is leaked}} expected-note{{Initialized va_list is leaked}}
+}
+
+void f5(va_list fst, ...) {
+ va_start(fst, fst);
+ //FIXME: this should cause a warning
+} // no-warning
+
+void f6(va_list *fst, ...) {
+ va_start(*fst, fst); // expected-note{{Initialized va_list}}
+ (void)va_arg(*fst, int);
+ //FIXME: this should NOT cause a warning
+ va_end(*fst); // expected-warning{{Initialized va_list is leaked}} expected-note{{Initialized va_list is leaked}}
+}
+
+void f7(int *fst, ...) {
+ va_list x;
+ va_list *y =
+ va_start(*y,fst); // expected-note{{Initialized va_list}}
+} // expected-warning{{Initialized va_list 'x' is leaked}} expected-note{{Initialized va_list 'x' is leaked}}
+
+void f8(int *fst, ...) {
+ va_list x;
+ va_list *y =
+ va_start(*y,fst);
+ va_end(x);
+} // no-warning
+
+void reinit(int *fst, ...) {
+ va_list va;
+ va_start(va, fst); // expected-note{{Initialized va_list}} expected-note{{Initialized va_list}}
+ va_start(va, fst); // expected-warning{{Initialized va_list 'va' is initialized again}} expected-note{{Initialized va_list 'va' is initialized again}}
+} // expected-warning{{Initialized va_list 'va' is leaked}} expected-note{{Initialized va_list 'va' is leaked}}
+
+void reinitOk(int *fst, ...) {
+ va_list va;
+ va_start(va, fst);
+ va_end(va);
+ va_start(va, fst);
+ va_end(va);
+} // no-warning
+
+void copyself(int fst, ...) {
+ va_list va;
+ va_start(va, fst); // expected-note{{Initialized va_list}}
+ va_copy(va, va); // expected-warning{{va_list 'va' is copied onto itself}} expected-note{{va_list 'va' is copied onto itself}}
+ va_end(va);
+} // no-warning
+
+void copyselfUninit(int fst, ...) {
+ va_list va;
+ va_copy(va, va); // expected-warning{{va_list 'va' is copied onto itself}} expected-note{{va_list 'va' is copied onto itself}}
+} // no-warning
+
+void copyOverwrite(int fst, ...) {
+ va_list va, va2;
+ va_start(va, fst); // expected-note{{Initialized va_list}}
+ va_copy(va, va2); // expected-warning{{Initialized va_list 'va' is overwritten by an uninitialized one}} expected-note{{Initialized va_list 'va' is overwritten by an uninitialized one}}
+} // no-warning
+
+//This only generates a warning for the valist.Uninitialized checker
+void copyUnint(int fst, ...) {
+ va_list va, va2;
+ va_copy(va, va2);
+} // no-warning
+
+void recopy(int fst, ...) {
+ va_list va, va2;
+ va_start(va, fst);
+ va_copy(va2, va); // expected-note{{Initialized va_list}}
+ va_copy(va2, va); // expected-warning{{Initialized va_list 'va2' is initialized again}} expected-note{{Initialized va_list 'va2' is initialized again}}
+ va_end(va);
+ va_end(va2);
+} //no-warning
+
+void doublemsg(int fst, ...) {
+ va_list va, va2;
+ va_start(va, fst), va_start(va2, fst); // expected-warning{{Initialized va_list 'va' is leaked}} expected-warning{{Initialized va_list 'va2' is leaked}} expected-note{{Initialized va_list}} expected-note{{Initialized va_list}} expected-note{{Initialized va_list}} expected-note{{Initialized va_list 'va' is leaked}}
+}
+
+void in_array(int fst, ...) {
+ va_list va_array[8];
+ va_start(va_array[3], fst); //
Re: [PATCH] D15227: [analyzer] Valist checkers.
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(Reg);
+ const auto *EReg = dyn_cast_or_null(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 Exit = P.getAs())
+ 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15227: [analyzer] Valist checkers.
dcoughlin added inline comments. 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); Are there tests for this diagnostic? I don't see any. https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15227: [analyzer] Valist checkers.
xazax.hun marked 10 inline comments as done. xazax.hun added a comment. https://reviews.llvm.org/D15227 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15227: [analyzer] Valist checkers.
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:31
@@ +30,3 @@
+struct VAListAcceptingFunc {
+ mutable IdentifierInfo *II;
+ StringRef FuncName;
It does not support ObjC methods.
I think this is most useful to checker writes, so how about placing it in
CheckerContext? (I am fine with CallEven as well if you think it has wider
applicability.)
Comment at: lib/StaticAnalyzer/Checkers/ValistChecker.cpp:205
@@ +204,3 @@
+ if (!TReg)
+return nullptr;
+ return TReg;
How about we commit your implementation as a general utility and point the
other review to it? You can make it as a separate commit for clarity.
Comment at: test/Analysis/valist-uninitialized.c:135
@@ +134,3 @@
+}
+
+// NOTE: this is invalid, as the man page of va_end requires that "Each
invocation of va_start()
Please, make that clear in the comment, for example, you can say "We should
produce a warning here..".
http://reviews.llvm.org/D15227
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15227: [analyzer] Valist checkers.
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, 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 , CheckerContext ) 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15227: [analyzer] Valist checkers.
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, 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 , CheckerContext ) 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, 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 ,
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D15227: [analyzer] Valist checkers.
xazax.hun created this revision.
xazax.hun added reviewers: dcoughlin, zaks.anna, jordan_rose.
xazax.hun added subscribers: dkrupp, cfe-commits.
This checker was developed by Donat Nagy (m1nag...@gmail.com) and myself.
It finds usages of uninitialized va_lists, and also unterminated va_lists.
http://reviews.llvm.org/D15227
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/ValistChecker.cpp
test/Analysis/Inputs/system-header-simulator-for-valist.h
test/Analysis/Inputs/system-header-simulator-valist.h
test/Analysis/valist-uninitialized.c
test/Analysis/valist-unterminated.c
Index: test/Analysis/valist-unterminated.c
===
--- /dev/null
+++ test/Analysis/valist-unterminated.c
@@ -0,0 +1,131 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.valist.Unterminated -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator-for-valist.h"
+
+void f1(int fst, ...) {
+ va_list va;
+ va_start(va, fst);
+ return; // expected-warning{{Initialized va_list 'va' is leaked}}
+}
+
+void f2(int fst, ...) {
+ va_list va;
+ va_start(va, fst);
+ va_end(va);
+ va_start(va, fst);
+} // expected-warning{{Initialized va_list 'va' is leaked}}
+
+void f3(int fst, ...) {
+ va_list va, va2;
+ va_start(va, fst);
+ va_copy(va2, va);
+ va_end(va); // expected-warning{{Initialized va_list 'va2' is leaked}}
+}
+
+void f4(va_list *fst, ...) {
+ va_start(*fst, fst);
+ return; // expected-warning{{Initialized va_list is leaked}}
+}
+
+void f5(va_list fst, ...) {
+ va_start(fst, fst);
+} // no-warning
+ //FIXME: this should cause a warning
+
+void f6(va_list *fst, ...) {
+ va_start(*fst, fst);
+ (void)va_arg(*fst, int);
+ va_end(*fst); // expected-warning{{Initialized va_list is leaked}}
+}
+ //FIXME: this should NOT cause a warning
+
+void f7(int *fst, ...) {
+ va_list x;
+ va_list *y =
+ va_start(*y,fst);
+} // expected-warning{{Initialized va_list 'x' is leaked}}
+
+void f8(int *fst, ...) {
+ va_list x;
+ va_list *y =
+ va_start(*y,fst);
+ va_end(x);
+} // no-warning
+
+void reinit(int *fst, ...) {
+ va_list va;
+ va_start(va, fst);
+ va_start(va, fst); // expected-warning{{Initialized va_list 'va' is initialized again}}
+} // expected-warning{{Initialized va_list 'va' is leaked}}
+
+void reinitOk(int *fst, ...) {
+ va_list va;
+ va_start(va, fst);
+ va_end(va);
+ va_start(va, fst);
+ va_end(va);
+} // no-warning
+
+void copyself(int fst, ...) {
+ va_list va;
+ va_start(va, fst);
+ va_copy(va, va); // expected-warning{{va_list 'va' is copied onto itself}}
+ va_end(va);
+} // no-warning
+
+void copyselfUninit(int fst, ...) {
+ va_list va;
+ va_copy(va, va); // expected-warning{{va_list 'va' is copied onto itself}}
+} // no-warning
+
+void copyOverwrite(int fst, ...) {
+ va_list va, va2;
+ va_start(va, fst);
+ va_copy(va, va2); // expected-warning{{Initialized va_list 'va' is overwritten by an uninitialized one}}
+} // no-warning
+
+//This only generates a warning for the valist.Uninitialized checker
+void copyUnint(int fst, ...) {
+ va_list va, va2;
+ va_copy(va, va2);
+} // no-warning
+
+void recopy(int fst, ...) {
+ va_list va, va2;
+ va_start(va, fst);
+ va_copy(va2, va);
+ va_copy(va2, va); // expected-warning{{Initialized va_list 'va2' is initialized again}}
+ va_end(va);
+ va_end(va2);
+} //no-warning
+
+void doublemsg(int fst, ...) {
+ va_list va, va2;
+ va_start(va, fst), va_start(va2, fst); // expected-warning{{Initialized va_list 'va' is leaked}} expected-warning{{Initialized va_list 'va2' is leaked}}
+}
+
+void in_array(int fst, ...) {
+ va_list va_array[8];
+ va_start(va_array[3], fst);
+} // expected-warning{{Initialized va_list 'va_array' is leaked}}
+
+struct containing_a_valist {
+ va_list vafield;
+ int foobar;
+};
+
+void in_struct(int fst, ...) {
+ struct containing_a_valist s;
+ va_start(s.vafield, fst);
+} // expected-warning{{Initialized va_list 'vafield' is leaked}}
+
+void casting(int fst, ...) {
+ char mem[sizeof(va_list)];
+ va_start(*(va_list *) mem, fst);
+} // expected-warning{{Initialized va_list 'mem' is leaked}}
+
+void castingOk(int fst, ...) {
+ char mem[sizeof(va_list)];
+ va_start(*(va_list *) mem, fst);
+ va_end(*(va_list *) mem);
+} // no-warning
Index: test/Analysis/valist-uninitialized.c
===
--- /dev/null
+++ test/Analysis/valist-uninitialized.c
@@ -0,0 +1,177 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.valist.Uninitialized -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator-for-valist.h"
+
+void f1(int fst, ...) {
+ va_list va;
+ (void)va_arg(va, int); //expected-warning{{va_arg() is called on an uninitialized va_list}}
+}
+
+int f2(int fst, ...) {
+ va_list va;
+ va_start(va, fst);
+ va_end(va);
+ return va_arg(va, int);
