[PATCH] D27918: [analyzer] OStreamChecker
gamesh411 abandoned this revision. gamesh411 added a comment. Herald added subscribers: Charusso, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware. There are multiple new additions to stream formatters, and this patch is way too old. I may consider creating a new checker like this with an extended feature set later. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D27918/new/ https://reviews.llvm.org/D27918 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27918: [analyzer] OStreamChecker
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:513
+
+bool OStreamFormatChecker::evalCall(const CallExpr *CE,
+CheckerContext &C) const {
gamesh411 wrote:
> NoQ wrote:
> > One of the practical differences between `checkPostCall` and `evalCall` is
> > that in the latter you have full control over the function execution,
> > including invalidations that the call causes. Your code not only sets the
> > return value, but also removes invalidations that normally happen. Like,
> > normally when an unknown function is called, it is either inlined and
> > therefore modeled directly, or destroys all information about any global
> > variables or heap memory that it might have touched. By implementing
> > `evalCall`, you are saying that the only effect of calling `operator<<()`
> > on a `basic_ostream` is returning the first argument lvalue, and nothing
> > else happens; inlining is suppressed, invalidation is suppressed.
> >
> > I'm not sure if it's a good thing. On one hand, this is not entirely true,
> > because the operator changes the internal state of the stream and maybe of
> > some global stuff inside the standard library. On the other hand, it is
> > unlikely to matter in practice, as far as i can see.
> >
> > Would it undermine any of your efforts here if you add a manual
> > invalidation of the stream object and of the `GlobalSystemSpaceRegion`
> > memory space (you could construct a temporary `CallEvent` and call one of
> > its methods if it turns out to be easier)?
> >
> > I'm not exactly in favor of turning this into `checkPostCall()` because
> > binding expressions in that callback may cause hard-to-notice conflicts
> > across multiple checkers. Some checkers may even take the old value before
> > it's replaced. For `evalCall()` we at least have an assert.
> >
> > If you intend to keep the return value as the only effect, there's option
> > of making a synthesized body in our body farm, which is even better at
> > avoiding inter-checker conflicts. Body farms were created for that specific
> > purpose, even though they also have their drawbacks (sometimes `evalCall`
> > is more flexible than anything we could synthesize, eg. D20811).
> >
> > If you have considered all the alternative options mentioned above and
> > rejected them, could you add a comment explaining why? :)
> I am not familiar with the BodyFarm approach, however I tried something
> along the lines of:
> auto CEvt =
> ResultEqualsFirstParam->getStateManager().getCallEventManager().getSimpleCall(CE,
> S, C.getLocationContext());
> ProgramStateRef StreamStateInvalidated =
> CEvt->invalidateRegions(C.blockCount());
>
> It however broke test2 (where the state is set to hex formatting, then, back
> to dec). Any tips why resetting regions could cause problems?
>
I agree that we should not use evalCall(), especially, in an opt-in checker, as
it can introduce subtle/hard to debug issues. As was mentioned, if a checker
implements evalCall(), the usual evaluation by the analyzer core does not
occur, which could lead to various unpredictable results.
https://reviews.llvm.org/D27918
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27918: [analyzer] OStreamChecker
gamesh411 added a comment. Ping. @NoQ would you please have a look? Thanks! https://reviews.llvm.org/D27918 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27918: [analyzer] OStreamChecker
gamesh411 updated this revision to Diff 107923.
https://reviews.llvm.org/D27918
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp
test/Analysis/Inputs/system-header-simulator-cxx-iostream.h
test/Analysis/ostream-format.cpp
Index: test/Analysis/ostream-format.cpp
===
--- /dev/null
+++ test/Analysis/ostream-format.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.OStreamFormat -verify -std=c++11 %s
+
+#include "Inputs/system-header-simulator-cxx-iostream.h"
+
+
+class StreamState {
+public:
+ StreamState(std::ostream &out)
+ : m_out(out), m_fmt(out.flags()), m_prec(out.precision()) {}
+
+ ~StreamState() {
+m_out.precision(m_prec);
+m_out.flags(m_fmt);
+ }
+
+private:
+ std::ostream &m_out;
+ std::ios_base::fmtflags m_fmt;
+ std::streamsize m_prec;
+};
+
+void test1(int i) {
+ std::cout << std::hex << i;
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+void test2(int i) { std::cout << std::hex << i << std::dec; } // OK.
+
+void test3(int i) {
+ std::cout << std::hex << i;
+ std::cout << std::dec;
+} // ok
+
+void test4(float f) {
+ std::cout << std::setprecision(2)
+<< f; // The stream state is chaged, but not restored.
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+int test5() {
+ std::cout.setf(std::ios::hex,
+ std::ios::basefield); // Set hex as the basefield.
+ std::cout.setf(std::ios::showbase); // Activating showbase.
+ std::cout << 100 << '\n';
+ std::cout.unsetf(std::ios::showbase); // Deactivating showbase.
+ std::cout << 100 << '\n';
+ return 0; // The stream state is chaged, but not restored.
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+void test6(const char *results) {
+ StreamState ss(std::cout);
+ std::cout << std::setiosflags(std::ios::fixed)
+<< std::setprecision(6); // The stream state is set here.
+ std::cout << results << std::endl;
+} // OK, stream state is recovered here.
+
+void test7(int i) {
+ std::cout << std::hex << i;
+ std::cout.setf(std::ios::dec);
+} // OK.
+
+void test8(float i) {
+ std::cout << std::setprecision(12) << i;
+ std::cout.precision(6);
+} // OK.
+
+void test9(float i) {
+ std::cout.precision(6);
+ std::cout << std::setprecision(12) << i;
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+void test10(float i) {
+ std::cout.precision(12);
+ std::cout << std::setprecision(6) << i;
+} // OK.
+
Index: test/Analysis/Inputs/system-header-simulator-cxx-iostream.h
===
--- /dev/null
+++ test/Analysis/Inputs/system-header-simulator-cxx-iostream.h
@@ -0,0 +1,195 @@
+// 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
+
+namespace std {
+typedef unsigned streamsize;
+
+namespace ios {
+int boolalpha;
+int dec;
+int fixed;
+int hex;
+int internal;
+int left;
+int oct;
+int right;
+int scientific;
+int showbase;
+int showpoint;
+int showpos;
+int skipws;
+int unitbuf;
+int uppercase;
+int adjustfield;
+int basefield;
+int floatfield;
+}
+
+class ios_base {
+public:
+ typedef int fmtflags;
+ fmtflags m_fmt;
+ fmtflags flags() const;
+ fmtflags flags(fmtflags);
+ fmtflags setf(fmtflags);
+ fmtflags setf(fmtflags, fmtflags);
+ void unsetf(fmtflags);
+ streamsize precision() const;
+ streamsize precision(streamsize);
+ streamsize width() const;
+ streamsize width(streamsize);
+};
+
+template class basic_ios : public ios_base {};
+
+// Simple manipulators.
+
+ios_base &boolalpha(ios_base &);
+ios_base &noboolalpha(ios_base &);
+ios_base &showbase(ios_base &);
+ios_base &noshowbase(ios_base &);
+ios_base &showpoint(ios_base &);
+ios_base &noshowpoint(ios_base &);
+ios_base &showpos(ios_base &);
+ios_base &noshowpos(ios_base &);
+ios_base &skipws(ios_base &);
+ios_base &noskipws(ios_base &);
+ios_base &uppercase(ios_base &);
+ios_base &nouppercase(ios_base &);
+ios_base &unitbuf(ios_base &);
+ios_base &nounitbuf(ios_base &);
+ios_base &internal(ios_base &);
+ios_base &left(ios_base &);
+ios_base &right(ios_base &);
+ios_base &dec(ios_base &);
+ios_base &hex(ios_base &);
+ios_base &oct(ios_base &);
+ios_base &fixed(ios_base &);
+ios_base &scientific(ios_base &);
+ios_base &hexfloat(ios_base &);
+ios_base &defaultfloat(ios_base &);
+
+
+template class basic_ostream : public basic_ios {
+public:
+ basic_ostream &operator<<(int);
+ basic_ostream &operator<<(float);
+ basic_ostream &operat
[PATCH] D27918: [analyzer] OStreamChecker
gamesh411 updated this revision to Diff 107921.
https://reviews.llvm.org/D27918
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp
test/Analysis/Inputs/system-header-simulator-cxx-iostream.h
test/Analysis/ostream-format.cpp
Index: test/Analysis/ostream-format.cpp
===
--- /dev/null
+++ test/Analysis/ostream-format.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.OStreamFormat -verify -std=c++11 %s
+
+#include "Inputs/system-header-simulator-cxx-iostream.h"
+
+
+class StreamState {
+public:
+ StreamState(std::ostream &out)
+ : m_out(out), m_fmt(out.flags()), m_prec(out.precision()) {}
+
+ ~StreamState() {
+m_out.precision(m_prec);
+m_out.flags(m_fmt);
+ }
+
+private:
+ std::ostream &m_out;
+ std::ios_base::fmtflags m_fmt;
+ std::streamsize m_prec;
+};
+
+void test1(int i) {
+ std::cout << std::hex << i;
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+void test2(int i) { std::cout << std::hex << i << std::dec; } // OK.
+
+void test3(int i) {
+ std::cout << std::hex << i;
+ std::cout << std::dec;
+} // ok
+
+void test4(float f) {
+ std::cout << std::setprecision(2)
+<< f; // The stream state is chaged, but not restored.
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+int test5() {
+ std::cout.setf(std::ios::hex,
+ std::ios::basefield); // Set hex as the basefield.
+ std::cout.setf(std::ios::showbase); // Activating showbase.
+ std::cout << 100 << '\n';
+ std::cout.unsetf(std::ios::showbase); // Deactivating showbase.
+ std::cout << 100 << '\n';
+ return 0; // The stream state is chaged, but not restored.
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+void test6(const char *results) {
+ StreamState ss(std::cout);
+ std::cout << std::setiosflags(std::ios::fixed)
+<< std::setprecision(6); // The stream state is set here.
+ std::cout << results << std::endl;
+} // OK, stream state is recovered here.
+
+void test7(int i) {
+ std::cout << std::hex << i;
+ std::cout.setf(std::ios::dec);
+} // OK.
+
+void test8(float i) {
+ std::cout << std::setprecision(12) << i;
+ std::cout.precision(6);
+} // OK.
+
+void test9(float i) {
+ std::cout.precision(6);
+ std::cout << std::setprecision(12) << i;
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+void test10(float i) {
+ std::cout.precision(12);
+ std::cout << std::setprecision(6) << i;
+} // OK.
+
Index: test/Analysis/Inputs/system-header-simulator-cxx-iostream.h
===
--- /dev/null
+++ test/Analysis/Inputs/system-header-simulator-cxx-iostream.h
@@ -0,0 +1,195 @@
+// 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
+
+namespace std {
+typedef unsigned streamsize;
+
+namespace ios {
+int boolalpha;
+int dec;
+int fixed;
+int hex;
+int internal;
+int left;
+int oct;
+int right;
+int scientific;
+int showbase;
+int showpoint;
+int showpos;
+int skipws;
+int unitbuf;
+int uppercase;
+int adjustfield;
+int basefield;
+int floatfield;
+}
+
+class ios_base {
+public:
+ typedef int fmtflags;
+ fmtflags m_fmt;
+ fmtflags flags() const;
+ fmtflags flags(fmtflags);
+ fmtflags setf(fmtflags);
+ fmtflags setf(fmtflags, fmtflags);
+ void unsetf(fmtflags);
+ streamsize precision() const;
+ streamsize precision(streamsize);
+ streamsize width() const;
+ streamsize width(streamsize);
+};
+
+template class basic_ios : public ios_base {};
+
+// Simple manipulators.
+
+ios_base &boolalpha(ios_base &);
+ios_base &noboolalpha(ios_base &);
+ios_base &showbase(ios_base &);
+ios_base &noshowbase(ios_base &);
+ios_base &showpoint(ios_base &);
+ios_base &noshowpoint(ios_base &);
+ios_base &showpos(ios_base &);
+ios_base &noshowpos(ios_base &);
+ios_base &skipws(ios_base &);
+ios_base &noskipws(ios_base &);
+ios_base &uppercase(ios_base &);
+ios_base &nouppercase(ios_base &);
+ios_base &unitbuf(ios_base &);
+ios_base &nounitbuf(ios_base &);
+ios_base &internal(ios_base &);
+ios_base &left(ios_base &);
+ios_base &right(ios_base &);
+ios_base &dec(ios_base &);
+ios_base &hex(ios_base &);
+ios_base &oct(ios_base &);
+ios_base &fixed(ios_base &);
+ios_base &scientific(ios_base &);
+ios_base &hexfloat(ios_base &);
+ios_base &defaultfloat(ios_base &);
+
+
+template class basic_ostream : public basic_ios {
+public:
+ basic_ostream &operator<<(int);
+ basic_ostream &operator<<(float);
+ basic_ostream &operat
[PATCH] D27918: [analyzer] OStreamChecker
gamesh411 added a comment.
Hello,
After experimentation the following AST difference between the mock and the
standard library implementation still stands (which necessitates the special
handling of the complex manipulators). Example:
// The different behaviour of the AST is illustrated on the test4 function of
the test suite:
//
// void test4(float f) {
// std::cout << std::setprecision(2)
// << f; // The stream state is chaged, but not restored.
// } // expected-warning{{Possibly forgotten ostream format modification in
scope}}
//
// The AST of the function with the standard library include
// (note the wrapping CXXConstructorExpr):
// |-FunctionDecl 0x5642e3f80b70 line:33:6 test4 'void
(float)'
// | |-ParmVarDecl 0x5642e3f80ab0 col:18 used f 'float'
// | `-CompoundStmt 0x5642e3f818b0
// | `-ExprWithCleanups 0x5642e3f81898
'std::basic_ostream
>::__ostream_type':'class std::basic_ostream' lvalue
// | `-CXXOperatorCallExpr 0x5642e3f81850
'std::basic_ostream
>::__ostream_type':'class std::basic_ostream' lvalue
// | |-ImplicitCastExpr 0x5642e3f81838
'std::basic_ostream >::__ostream_type
&(*)(float)'
// | | `-DeclRefExpr 0x5642e3f817b8 'std::basic_ostream >::__ostream_type &(float)' lvalue CXXMethod
0x5642e3edf120 'operator<<' 'std::basic_ostream >::__ostream_type &(float)'
// | |-CXXOperatorCallExpr 0x5642e3f81270
'basic_ostream >':'class
std::basic_ostream' lvalue
// | | |-ImplicitCastExpr 0x5642e3f81258 'basic_ostream > &(*)(basic_ostream > &, struct std::_Setprecision)'
// | | | `-DeclRefExpr 0x5642e3f811d0 'basic_ostream > &(basic_ostream > &, struct std::_Setprecision)' lvalue Function
0x5642e3f6a6e0 'operator<<' 'basic_ostream
> &(basic_ostream > &, struct
std::_Setprecision)'
// | | |-DeclRefExpr 0x5642e3f80c30
'std::ostream':'class std::basic_ostream' lvalue Var 0x5642e3f56578
'cout' 'std::ostream':'class std::basic_ostream'
// | | `-CXXConstructExpr 0x5642e3f81198 'struct
std::_Setprecision' 'void (const struct std::_Setprecision &) throw()' elidable
// | | `-MaterializeTemporaryExpr 0x5642e3f81088
'const struct std::_Setprecision' lvalue
// | | `-ImplicitCastExpr 0x5642e3f81070 'const
struct std::_Setprecision'
// | | `-CallExpr 0x5642e3f80d20 'struct
std::_Setprecision'
// | | |-ImplicitCastExpr 0x5642e3f80d08
'struct std::_Setprecision (*)(int)'
// | | | `-DeclRefExpr 0x5642e3f80c88 'struct
std::_Setprecision (int)' lvalue Function 0x5642e3f63f70 'setprecision' 'struct
std::_Setprecision (int)'
// | | `-IntegerLiteral 0x5642e3f80cc0 'int' 2
// | `-ImplicitCastExpr 0x5642e3f817a0 'float'
// | `-DeclRefExpr 0x5642e3f812b8 'float' lvalue ParmVar
0x5642e3f80ab0 'f' 'float'
//
| | `-IntegerLiteral
0x5642e3f80cc0 'int' 2
// The AST of the function with the mock library include
// |-FunctionDecl 0x55c053dfaa70 line:32:6 test4 'void
(float)'
// | |-ParmVarDecl 0x55c053dfa9b0 col:18 used f 'float'
// | `-CompoundStmt 0x55c053dfb7c0
// | `-ExprWithCleanups 0x55c053dfb7a8 'class
std::basic_ostream' lvalue
// | `-CXXOperatorCallExpr 0x55c053dfb760 'class
std::basic_ostream' lvalue
// | |-ImplicitCastExpr 0x55c053dfb748 'class
std::basic_ostream &(*)(float)'
// | | `-DeclRefExpr 0x55c053dfb6f8 'class
std::basic_ostream &(float)' lvalue CXXMethod 0x55c053df8120 'operator<<'
'class std::basic_ostream &(float)'
// | |-CXXOperatorCallExpr 0x55c053dfb670
'basic_ostream':'class std::basic_ostream' lvalue
// | | |-ImplicitCastExpr 0x55c053dfb658 'basic_ostream
&(*)(basic_ostream &, const class std::setprecision_manip &)'
// | |
[PATCH] D27918: [analyzer] OStreamChecker
gamesh411 updated this revision to Diff 101549.
gamesh411 marked an inline comment as done.
gamesh411 added a comment.
Update diff.
https://reviews.llvm.org/D27918
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp
test/Analysis/Inputs/system-header-simulator-cxx-iostream.h
test/Analysis/ostream-format.cpp
Index: test/Analysis/ostream-format.cpp
===
--- /dev/null
+++ test/Analysis/ostream-format.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.OStreamFormat -verify -std=c++11 %s
+
+#include "Inputs/system-header-simulator-cxx-iostream.h"
+
+
+class StreamState {
+public:
+ StreamState(std::ostream &out)
+ : m_out(out), m_fmt(out.flags()), m_prec(out.precision()) {}
+
+ ~StreamState() {
+m_out.precision(m_prec);
+m_out.flags(m_fmt);
+ }
+
+private:
+ std::ostream &m_out;
+ std::ios_base::fmtflags m_fmt;
+ std::streamsize m_prec;
+};
+
+void test1(int i) {
+ std::cout << std::hex << i;
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+void test2(int i) { std::cout << std::hex << i << std::dec; } // OK.
+
+void test3(int i) {
+ std::cout << std::hex << i;
+ std::cout << std::dec;
+} // ok
+
+void test4(float f) {
+ std::cout << std::setprecision(2)
+<< f; // The stream state is chaged, but not restored.
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+int test5() {
+ std::cout.setf(std::ios::hex,
+ std::ios::basefield); // Set hex as the basefield.
+ std::cout.setf(std::ios::showbase); // Activating showbase.
+ std::cout << 100 << '\n';
+ std::cout.unsetf(std::ios::showbase); // Deactivating showbase.
+ std::cout << 100 << '\n';
+ return 0; // The stream state is chaged, but not restored.
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+void test6(std::string &results) {
+ StreamState ss(std::cout);
+ std::cout << std::setiosflags(std::ios::fixed)
+<< std::setprecision(6); // The stream state is set here.
+ std::cout << results << std::endl;
+} // OK, stream state is recovered here.
+
+void test7(int i) {
+ std::cout << std::hex << i;
+ std::cout.setf(std::ios::dec);
+} // OK.
+
+void test8(float i) {
+ std::cout << std::setprecision(12) << i;
+ std::cout.precision(6);
+} // OK.
+
+void test9(float i) {
+ std::cout.precision(6);
+ std::cout << std::setprecision(12) << i;
+} // expected-warning{{Possibly forgotten ostream format modification in scope}}
+
+void test10(float i) {
+ std::cout.precision(12);
+ std::cout << std::setprecision(6) << i;
+} // OK.
+
Index: test/Analysis/Inputs/system-header-simulator-cxx-iostream.h
===
--- /dev/null
+++ test/Analysis/Inputs/system-header-simulator-cxx-iostream.h
@@ -0,0 +1,195 @@
+// 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
+
+namespace std {
+typedef unsigned streamsize;
+
+namespace ios {
+int boolalpha;
+int dec;
+int fixed;
+int hex;
+int internal;
+int left;
+int oct;
+int right;
+int scientific;
+int showbase;
+int showpoint;
+int showpos;
+int skipws;
+int unitbuf;
+int uppercase;
+int adjustfield;
+int basefield;
+int floatfield;
+}
+
+class ios_base {
+public:
+ typedef int fmtflags;
+ fmtflags m_fmt;
+ fmtflags flags() const;
+ fmtflags flags(fmtflags);
+ fmtflags setf(fmtflags);
+ fmtflags setf(fmtflags, fmtflags);
+ void unsetf(fmtflags);
+ streamsize precision() const;
+ streamsize precision(streamsize);
+ streamsize width() const;
+ streamsize width(streamsize);
+};
+
+template class basic_ios : public ios_base {};
+
+// Simple manipulators.
+
+ios_base &boolalpha(ios_base &);
+ios_base &noboolalpha(ios_base &);
+ios_base &showbase(ios_base &);
+ios_base &noshowbase(ios_base &);
+ios_base &showpoint(ios_base &);
+ios_base &noshowpoint(ios_base &);
+ios_base &showpos(ios_base &);
+ios_base &noshowpos(ios_base &);
+ios_base &skipws(ios_base &);
+ios_base &noskipws(ios_base &);
+ios_base &uppercase(ios_base &);
+ios_base &nouppercase(ios_base &);
+ios_base &unitbuf(ios_base &);
+ios_base &nounitbuf(ios_base &);
+ios_base &internal(ios_base &);
+ios_base &left(ios_base &);
+ios_base &right(ios_base &);
+ios_base &dec(ios_base &);
+ios_base &hex(ios_base &);
+ios_base &oct(ios_base &);
+ios_base &fixed(ios_base &);
+ios_base &scientific(ios_base &);
+ios_base &hexfloat(ios_base &);
+ios_base &defaultfloat(ios_base &);
+
+
+template class basic_ostream : public basic_ios {
+public:
+ basic_
[PATCH] D27918: [analyzer] OStreamChecker
gamesh411 marked 6 inline comments as done.
gamesh411 added a comment.
Update diff.
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:263-282
+ mutable IdentifierInfo *II_BasicOstream, *II_Flags, *II_Setf, *II_Unsetf,
+ *II_Setiosflags, *II_Resetiosflags, *II_Precision, *II_SetPrecision,
+ *II_BaseField, *II_Hex, *II_Dec, *II_Oct, *II_AdjustField, *II_Left,
+ *II_Right, *II_Internal, *II_BoolAlpha, *II_NoBoolAlpha, *II_ShowPos,
+ *II_NoShowPos, *II_ShowBase, *II_NoShowBase, *II_UpperCase,
+ *II_NoUpperCase, *II_ShowPoint, *II_NoShowPoint, *II_FloatField,
+ *II_Fixed, *II_Scientific;
NoQ wrote:
> If you ever want to extend the `CallDescription` class to cover your use
> case, please let us know :)
>
> While most of these aren't functions, the approach used in this class could
> be used here as well - making initialization code shorter.
I will look at the CallDescription class, and how the checker can benefit. Is
it still a problem, that we can not initialize IdentifierInfos during checker
construction? If so, how will would a CallDescription implementation solve that?
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:387
+
+static Optional tryEvaluateAsInt(const Expr *E, ProgramStateRef S,
+ CheckerContext C) {
NoQ wrote:
> I think you should use `SimpleSValBuilder::getKnownValue()`. The
> `EvaluateAsInt` part doesn't add much because the analyzer's engine should
> already be more powerful than that. On the other hand, the
> `ConstraintManager::getSymVal()` thing, which is already done in
> `SimpleSValBuilder::getKnownValue()`, may be useful.
I have tried an implementation of getKnownValue(), and it more terse, and did
not handle the cases it should have had to anyway.
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:513
+
+bool OStreamFormatChecker::evalCall(const CallExpr *CE,
+CheckerContext &C) const {
NoQ wrote:
> One of the practical differences between `checkPostCall` and `evalCall` is
> that in the latter you have full control over the function execution,
> including invalidations that the call causes. Your code not only sets the
> return value, but also removes invalidations that normally happen. Like,
> normally when an unknown function is called, it is either inlined and
> therefore modeled directly, or destroys all information about any global
> variables or heap memory that it might have touched. By implementing
> `evalCall`, you are saying that the only effect of calling `operator<<()` on
> a `basic_ostream` is returning the first argument lvalue, and nothing else
> happens; inlining is suppressed, invalidation is suppressed.
>
> I'm not sure if it's a good thing. On one hand, this is not entirely true,
> because the operator changes the internal state of the stream and maybe of
> some global stuff inside the standard library. On the other hand, it is
> unlikely to matter in practice, as far as i can see.
>
> Would it undermine any of your efforts here if you add a manual invalidation
> of the stream object and of the `GlobalSystemSpaceRegion` memory space (you
> could construct a temporary `CallEvent` and call one of its methods if it
> turns out to be easier)?
>
> I'm not exactly in favor of turning this into `checkPostCall()` because
> binding expressions in that callback may cause hard-to-notice conflicts
> across multiple checkers. Some checkers may even take the old value before
> it's replaced. For `evalCall()` we at least have an assert.
>
> If you intend to keep the return value as the only effect, there's option of
> making a synthesized body in our body farm, which is even better at avoiding
> inter-checker conflicts. Body farms were created for that specific purpose,
> even though they also have their drawbacks (sometimes `evalCall` is more
> flexible than anything we could synthesize, eg. D20811).
>
> If you have considered all the alternative options mentioned above and
> rejected them, could you add a comment explaining why? :)
I am not familiar with the BodyFarm approach, however I tried something along
the lines of:
auto CEvt =
ResultEqualsFirstParam->getStateManager().getCallEventManager().getSimpleCall(CE,
S, C.getLocationContext());
ProgramStateRef StreamStateInvalidated =
CEvt->invalidateRegions(C.blockCount());
It however broke test2 (where the state is set to hex formatting, then, back
to dec). Any tips why resetting regions could cause problems?
https://reviews.llvm.org/D27918
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27918: [analyzer] OStreamChecker
NoQ added a comment.
Hello,
Sorry again for the delays, thank you for your patience. Your checker is in
good shape, very well-structured and easy to follow, you definitely know your
stuff, and my comments here are relatively minor.
Are you planning on making more varied warning messages, eg. specifying which
format modifiers are forgotten, and where in the code they were previously set?
The latter could be done by using the "bug reporter visitor" mechanism.
I'm sure we can at the very least eventually land this checker into the `optin`
package, which is the new package for warnings that aren't on by default, but
are advised to be turned on by the users if the codebase intends to use certain
language or API features or contracts (unlike the `alpha` package which is for
checkers undergoing development to be ultimately moved out of alpha). For
moving out of alpha, i think the work on the warning messages needs to be done,
and the checker needs to be tested on a large codebase to make sure that the
false positive rate is low (i guess you already did it), and that's pretty much
it :) On a side note, i've been recently thinking of making `optin` checkers
more visible to the `scan-build` users, eg. mentioning in the log that certain
checks may be worth enabling.
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:263-282
+ mutable IdentifierInfo *II_BasicOstream, *II_Flags, *II_Setf, *II_Unsetf,
+ *II_Setiosflags, *II_Resetiosflags, *II_Precision, *II_SetPrecision,
+ *II_BaseField, *II_Hex, *II_Dec, *II_Oct, *II_AdjustField, *II_Left,
+ *II_Right, *II_Internal, *II_BoolAlpha, *II_NoBoolAlpha, *II_ShowPos,
+ *II_NoShowPos, *II_ShowBase, *II_NoShowBase, *II_UpperCase,
+ *II_NoUpperCase, *II_ShowPoint, *II_NoShowPoint, *II_FloatField,
+ *II_Fixed, *II_Scientific;
If you ever want to extend the `CallDescription` class to cover your use case,
please let us know :)
While most of these aren't functions, the approach used in this class could be
used here as well - making initialization code shorter.
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:387
+
+static Optional tryEvaluateAsInt(const Expr *E, ProgramStateRef S,
+ CheckerContext C) {
I think you should use `SimpleSValBuilder::getKnownValue()`. The
`EvaluateAsInt` part doesn't add much because the analyzer's engine should
already be more powerful than that. On the other hand, the
`ConstraintManager::getSymVal()` thing, which is already done in
`SimpleSValBuilder::getKnownValue()`, may be useful.
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:403
+
+ return Optional();
+}
By the way, there's the `None` thing that represents any empty optional.
Comment at: lib/StaticAnalyzer/Checkers/OStreamFormatChecker.cpp:513
+
+bool OStreamFormatChecker::evalCall(const CallExpr *CE,
+CheckerContext &C) const {
One of the practical differences between `checkPostCall` and `evalCall` is that
in the latter you have full control over the function execution, including
invalidations that the call causes. Your code not only sets the return value,
but also removes invalidations that normally happen. Like, normally when an
unknown function is called, it is either inlined and therefore modeled
directly, or destroys all information about any global variables or heap memory
that it might have touched. By implementing `evalCall`, you are saying that the
only effect of calling `operator<<()` on a `basic_ostream` is returning the
first argument lvalue, and nothing else happens; inlining is suppressed,
invalidation is suppressed.
I'm not sure if it's a good thing. On one hand, this is not entirely true,
because the operator changes the internal state of the stream and maybe of some
global stuff inside the standard library. On the other hand, it is unlikely to
matter in practice, as far as i can see.
Would it undermine any of your efforts here if you add a manual invalidation of
the stream object and of the `GlobalSystemSpaceRegion` memory space (you could
construct a temporary `CallEvent` and call one of its methods if it turns out
to be easier)?
I'm not exactly in favor of turning this into `checkPostCall()` because binding
expressions in that callback may cause hard-to-notice conflicts across multiple
checkers. Some checkers may even take the old value before it's replaced. For
`evalCall()` we at least have an assert.
If you intend to keep the return value as the only effect, there's option of
making a synthesized body in our body farm, which is even better at avoiding
inter-checker conflicts. Body farms were created for that specific purpose,
even though they also have their drawbacks (sometimes `evalCall` is more
flexible than anything we could synth
[PATCH] D27918: [analyzer] OStreamChecker
gamesh411 added a comment. Hello, This checker was developed indeed with internal usage in mind. It should not necessary be added as a default checker. However I have run it on the boost-1.63.0 codebase, and there some some mildly interesting findings in examples and tests. There is also a true positive result in the core codebase. As for the patter described above, it certainly is a possible use-case to set streams inside functions, but very few ( if any ) false positives was found thanks to that, and the main case for them was that I could not run the cross-TU analyzer (boosts bjam build system proved to be reluctant to work together with ClangSA). https://reviews.llvm.org/D27918 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27918: [analyzer] OStreamChecker
NoQ added a comment.
Hello,
Thanks for the patch, and for the impressing amount of work you put into this.
Because the patch is huge, so it's going to take some time for somebody to
understand how everything works.
I'd probably like to learn a bit more about the motivation behind the checker.
You must be working on a project that uses a lot of `std` streams, and the
checker seems to verify a particular coding guideline you're following (revert
all changes to the stream locally). I wonder how universally useful this
guideline is. For instance, there might exist a project that enjoys writing
code like this:
void set_my_favorite_format_specifiers() {
std::cout << std::setprecision(12) << std::hex; // warning?
}
void revert_my_favorite_format_specifiers() {
std::cout << std::setprecision(6) << std::dec;
}
void foo() {
set_my_favorite_format_specifiers();
std::cout << 100 << std::endl;
revert_my_favorite_format_specifiers();
}
Do you expect such pattern to be common? If we enable the checker by default,
would positives on such code cause annoyance, and would there be a way to
suppress them?
https://reviews.llvm.org/D27918
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
