[PATCH] D32350: [Analyzer] Exception checker for misuse: uncaught/noncompliant throws

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware abandoned this revision.
baloghadamsoftware added a comment.

Superseded by https://reviews.llvm.org/D33537


https://reviews.llvm.org/D32350



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32350: [Analyzer] Exception checker for misuse: uncaught/noncompliant throws

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

xazax.hun added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:105
+  ReportExnSpec(ReportExnSpec) {
+  // In the beginning we have to parse list formatted options
+  SmallVector EnabledFuncsVec;

All comments should be full sentences starting with a capital letter and ending 
with a period. 



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:109
+  EnabledFuncs.insert(EnabledFuncsVec.begin(), EnabledFuncsVec.end());
+  EnabledFuncs.insert("swap");
+  EnabledFuncs.insert("main");

whisperity wrote:
> Why is `swap` hardcoded as an "enabledfunc"?
It is always possible to implement swap in a non-throwing way, and some 
implementations that are using the copy and swap idiom, expecting swap to be 
no-throw. 



Comment at: test/Analysis/exception-misuse.cpp:26
+
+~Y() {  // expected-warning{{This function should not throw}}
+// nesting

whisperity wrote:
> whisperity wrote:
> > Yet again, better wording: _Destructor not marked `noexcept(false)` should 
> > not throw_ (this is true since `C++11`, maybe this needs to be based on a 
> > conditional in the checker!)
> > 
> > @xazax.hun, any idea on what a good error message here should be?
> Also, a test case for a throwing, and `noexcept(false)`-specified dtor is 
> missing.
In fact,  the function can throw, if the exception is catched before leaving 
the function body. And in case the function does not throw but a called 
function do, that is also an error. So maybe something like exception are not 
allowed to leave this function? 


https://reviews.llvm.org/D32350



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32350: [Analyzer] Exception Checker

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:80
+  bool WasReThrow;
+  Types CurrentThrows; // actually alive exceptions
+  FunctionExceptionsMap

There are some comment formatting issues along these lines.



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:84
+  std::vector CallStack;// trace call hierarchy
+  SmallSet EnabledFuncs;  // allowed functions
+  SmallSet IgnoredExceptions; // ignored exceptions

I had to stop here for a moment and heavily think what this variable (and the 
relevant command-line argument) is used for.

Maybe this calls for a comment then. What is "allowed function"? One that is 
**explicitly** allowed to throw, based on the user's decision? This should be 
explained here.



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:109
+  EnabledFuncs.insert(EnabledFuncsVec.begin(), EnabledFuncsVec.end());
+  EnabledFuncs.insert("swap");
+  EnabledFuncs.insert("main");

Why is `swap` hardcoded as an "enabledfunc"?



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:329
+  const FunctionDecl *callee = C->getDirectCallee();
+  // if is not exist body for this function we do not care about
+  if (!callee || !callee->hasBody()) {

The phrasing should be fixed here for easier understanding.



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:336
+  FunctionExceptionsMap::const_iterator fnIt = ExceptionsThrown.find(name);
+  // already processed?
+  if (fnIt == ExceptionsThrown.end()) {

`already processed` what? A given exception type from a given function?



Comment at: lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp:468
+  const auto *D = pNode.get();
+  if (D == nullptr)
+return false;

`if (!D)`



Comment at: test/Analysis/exception-misuse.cpp:18
+
+Y(Y&&) { // expected-warning{{This function should not throw}}
+throw data;

I would use a much more descriptive error message here. E.g., explicitly say, 
that `move (constructor|operator=) should not throw`.



Comment at: test/Analysis/exception-misuse.cpp:26
+
+~Y() {  // expected-warning{{This function should not throw}}
+// nesting

Yet again, better wording: _Destructor not marked `noexcept(false)` should not 
throw_ (this is true since `C++11`, maybe this needs to be based on a 
conditional in the checker!)

@xazax.hun, any idea on what a good error message here should be?



Comment at: test/Analysis/exception-misuse.cpp:26
+
+~Y() {  // expected-warning{{This function should not throw}}
+// nesting

whisperity wrote:
> Yet again, better wording: _Destructor not marked `noexcept(false)` should 
> not throw_ (this is true since `C++11`, maybe this needs to be based on a 
> conditional in the checker!)
> 
> @xazax.hun, any idea on what a good error message here should be?
Also, a test case for a throwing, and `noexcept(false)`-specified dtor is 
missing.


https://reviews.llvm.org/D32350



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32350: [Analyzer] Exception Checker

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware created this revision.
Herald added a subscriber: mgorny.

This is an old checker used only internally until now. The original author is 
Bence Babati, I added him as a subscriber.

The checker checks whather exceptions escape the main() function, destructors. 
functions with exception specifications not containing the exception or 
functions specially marked by an option.

I did not change the name of the checker, but maybe ExceptionEscape or 
UncaughtException would be more suitable.

I am not sure whether Clang Static Analyzer is the right place for this checker 
since it only walks the AST. Maybe it should be reimplemented Clang-Tidy, but 
there we would need a new matcher that walks the call chain recursively. (As 
far as I know, we cannot write iterative matcher expressions.)


https://reviews.llvm.org/D32350

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/ExceptionMisuseChecker.cpp
  test/Analysis/exception-misuse.cpp

Index: test/Analysis/exception-misuse.cpp
===
--- /dev/null
+++ test/Analysis/exception-misuse.cpp
@@ -0,0 +1,247 @@
+// RUN: %clang_analyze_cc1 -std=c++11 -fcxx-exceptions -analyzer-checker=core,cplusplus,alpha.cplusplus.ExceptionMisuse -analyzer-config alpha.cplusplus.ExceptionMisuse:enabled_functions="test2;test3;test4;f;g" -analyzer-config alpha.cplusplus.ExceptionMisuse:ignored_exceptions="E1;E2" %s -verify
+
+struct X {
+~X() {
+try {
+try {
+} catch (int& a) {
+throw 42;
+}
+} catch (...) {
+}
+}
+};
+
+struct Y {
+char data;
+
+Y(Y&&) { // expected-warning{{This function should not throw}}
+throw data;
+}
+
+Y& operator=(Y&&) { // expected-warning{{This function should not throw}}
+throw data;
+}
+
+~Y() {  // expected-warning{{This function should not throw}}
+// nesting
+if (data == 'A') throw data;
+bar();
+}
+void bar();
+};
+
+struct MyExceptionBase {};
+struct MyException : public MyExceptionBase {};
+
+void control_func() {
+throw 1024;
+}
+void dummy_func();
+void proxy_func() {
+control_func();
+throw "MyException";
+}
+
+struct Z {
+void control() { throw 'Z'; }
+
+~Z() { // expected-warning{{This function should not throw}}
+try {
+proxy_func();
+} catch (int& a) {
+} catch (...) {
+throw;
+}
+}
+};
+
+void testFn() {
+try {
+try {
+try {
+MyException myexce;
+throw myexce;
+} catch (int a) {
+dummy_func();
+throw;
+}
+} catch (MyException m) {
+}
+} catch (...) {
+}
+}
+
+struct base {};
+struct buffer : public base {};
+
+struct P {
+~P() {
+try {
+testFn();
+throw 44;
+} catch (int& a) {
+} catch (...) {
+}
+}
+};
+
+struct V {
+~V() { // expected-warning{{This function should not throw}}
+try {
+try {
+MyException myexce;
+throw myexce;
+} catch (int a) {
+dummy_func();
+}
+} catch (const MyExceptionBase& m) {
+throw;
+} catch (buffer) {
+} catch (base) {
+proxy_func();
+} catch (int& a) {
+} catch (int* a) {
+} catch (...) {
+}
+}
+};
+
+struct W {
+~W() { // expected-warning{{This function should not throw}}
+try {
+try {
+int a = 44;
+throw a;
+double d = 4.44;
+throw d;
+testFn();
+} catch (...) {
+throw;
+}
+} catch (int a) {
+
+} catch (...) {
+throw;
+}
+}
+};
+
+struct Q {
+Q(bool b) { _b = b; }
+~Q() {
+try {
+buffer b;
+if (_b) throw b;
+} catch (...) {
+}
+}
+
+  private:
+bool _b;
+};
+
+namespace N {
+void swap(int&, int&) { throw 5; } // expected-warning{{This function should not throw}}
+
+struct S {
+void swap(S& other) { throw 5; } // expected-warning{{This function should not throw}}
+};
+}
+
+int test1() throw() { // expected-warning{{This function should not throw}}
+throw 7;
+return 0;
+}
+
+void fun2();
+int test2() { // expected-warning{{This function should not throw}}
+fun2();
+return 0;
+}
+void fun2() { throw 7; }
+
+namespace test3Ns {
+  void fun3() { throw 7; }
+  void f() { fun3(); } // expected-warning{{This function should not throw}}
+}
+
+class A {};
+class B {};
+class C {};
+
+int test4() { // expected-warning{{This function should not throw}}
+try {
+throw A();
+} catch (B