[PATCH] D33825: [clang-tidy] signal handler must be plain old function check
bruntib updated this revision to Diff 288777.
bruntib added a comment.
Note texts are now describing what kind of C++ constructs were used. The
warning message also explains better the reason of the issue: "signal handlers
must be plain old functions, C++-only constructs are not allowed"
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D33825/new/
https://reviews.llvm.org/D33825
Files:
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
clang-tools-extra/clang-tidy/cert/CMakeLists.txt
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
@@ -0,0 +1,134 @@
+// RUN: %check_clang_tidy %s cert-msc54-cpp %t -- -- -std=c++11
+
+namespace std {
+extern "C" using signalhandler = void(int);
+int signal(int sig, signalhandler *func);
+}
+
+#define SIG_ERR -1
+#define SIGTERM 15
+
+static void sig_handler(int sig) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: signal handlers must be 'extern "C"' [cert-msc54-cpp]
+// CHECK-MESSAGES: :[[@LINE+3]]:39: note: given as a signal parameter here
+
+void install_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, sig_handler))
+return;
+}
+
+extern "C" void cpp_signal_handler(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: signal handlers must be plain old functions, C++-only constructs are not allowed [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: throw statement used here
+ throw "error message";
+}
+
+void install_cpp_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, cpp_signal_handler))
+return;
+}
+
+void indirect_recursion();
+
+void cpp_like(){
+ try {
+cpp_signal_handler(SIG_ERR);
+ } catch(...) {
+// handle error
+ }
+}
+
+void no_body();
+
+void recursive_function() {
+ indirect_recursion();
+ cpp_like();
+ no_body();
+ recursive_function();
+}
+
+void indirect_recursion() {
+ recursive_function();
+}
+
+extern "C" void signal_handler_with_cpp_function_call(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE-11]]:3: note: function called here
+ // CHECK-MESSAGES: :[[@LINE-23]]:3: note: try statement used here
+ recursive_function();
+}
+
+void install_signal_handler_with_cpp_function_call() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_function_call))
+return;
+}
+
+class TestClass {
+public:
+ static void static_function() {}
+};
+
+extern "C" void signal_handler_with_cpp_static_method_call(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+2]]:3: note: function called here
+ // CHECK-MESSAGES: TestClass::static_function();
+ TestClass::static_function();
+}
+
+void install_signal_handler_with_cpp_static_method_call() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_static_method_call))
+return;
+}
+
+
+// Something that does not trigger the check:
+
+extern "C" void c_sig_handler(int sig) {
+#define cbrt(X) _Generic((X), long double \
+ : cbrtl, default \
+ : cbrt)(X)
+ auto char c = '1';
+ extern _Thread_local double _Complex d;
+ static const unsigned long int i = sizeof(float);
+ void f();
+ volatile struct c_struct {
+enum e {};
+union u;
+ };
+ typedef bool boolean;
+label:
+ switch (c) {
+ case ('1'):
+break;
+ default:
+d = 1.2;
+ }
+ goto label;
+ for (; i < 42;) {
+if (d == 1.2)
+ continue;
+else
+ return;
+ }
+ do {
+_Atomic int v = _Alignof(char);
+_Static_assert(42 == 42, "True");
+ } while (c == 42);
+}
+
+void install_c_sig_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, c_sig_handler)) {
+// Handle error
+ }
+}
+
+extern "C" void signal_handler_with_function_pointer(int sig) {
+ void (*funp) (void);
+ funp = _like;
+ funp();
+}
+
+void install_signal_handler_with_function_pointer() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_function_pointer))
+return;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@
[PATCH] D33825: [clang-tidy] signal handler must be plain old function check
bruntib added inline comments.
Comment at:
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:22
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not use C++ constructs in
signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: C++ construct used here
+ throw "error message";
jfb wrote:
> "C++ construct" isn't particularly useful. Here it needs to call out throwing
> exceptions.
Thanks for the review!
I think the clang-tidy message is understandable, because the next line after
"note: C++ construct used here" displays the exact source location where this
C++ construct is used. I included this line in the test file so it is visible
here. The full message looks like this:
llvm-project/clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:20:17:
warning: do not use C++ constructs in signal handlers [cert-msc54-cpp]
extern "C" void cpp_signal_handler(int sig) {
^
llvm-project/clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:23:3:
note: C++ construct used here
throw "error message";
^
Do you think it's enough, or should the note text be different for all C++
constructs?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D33825/new/
https://reviews.llvm.org/D33825
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33825: [clang-tidy] signal handler must be plain old function check
bruntib updated this revision to Diff 288774.
bruntib added a comment.
Test file has been extended with the second line of the "note" diagnostic
message which designates the location of the suspicious "C++ construct". The
full clang-tidy output looks like this:
llvm-project/clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:20:17:
warning: do not use C++ constructs in signal handlers [cert-msc54-cpp]
extern "C" void cpp_signal_handler(int sig) {
^
llvm-project/clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:23:3:
note: C++ construct used here
throw "error message";
^
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D33825/new/
https://reviews.llvm.org/D33825
Files:
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
clang-tools-extra/clang-tidy/cert/CMakeLists.txt
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
@@ -0,0 +1,136 @@
+// RUN: %check_clang_tidy %s cert-msc54-cpp %t -- -- -std=c++11
+
+namespace std {
+extern "C" using signalhandler = void(int);
+int signal(int sig, signalhandler *func);
+}
+
+#define SIG_ERR -1
+#define SIGTERM 15
+
+static void sig_handler(int sig) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: signal handlers must be 'extern "C"' [cert-msc54-cpp]
+// CHECK-MESSAGES: :[[@LINE+3]]:39: note: given as a signal parameter here
+
+void install_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, sig_handler))
+return;
+}
+
+extern "C" void cpp_signal_handler(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not use C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+2]]:3: note: C++ construct used here
+ // CHECK-MESSAGES: throw "error message";
+ throw "error message";
+}
+
+void install_cpp_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, cpp_signal_handler))
+return;
+}
+
+void indirect_recursion();
+
+void cpp_like(){
+ try {
+cpp_signal_handler(SIG_ERR);
+ } catch(...) {
+// handle error
+ }
+}
+
+void no_body();
+
+void recursive_function() {
+ indirect_recursion();
+ cpp_like();
+ no_body();
+ recursive_function();
+}
+
+void indirect_recursion() {
+ recursive_function();
+}
+
+extern "C" void signal_handler_with_cpp_function_call(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE-11]]:3: note: function called here
+ // CHECK-MESSAGES: :[[@LINE-23]]:3: note: C++ construct used here
+ // CHECK-MESSAGES: try {
+ recursive_function();
+}
+
+void install_signal_handler_with_cpp_function_call() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_function_call))
+return;
+}
+
+class TestClass {
+public:
+ static void static_function() {}
+};
+
+extern "C" void signal_handler_with_cpp_static_method_call(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+2]]:3: note: function called here
+ // CHECK-MESSAGES: TestClass::static_function();
+ TestClass::static_function();
+}
+
+void install_signal_handler_with_cpp_static_method_call() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_static_method_call))
+return;
+}
+
+
+// Something that does not trigger the check:
+
+extern "C" void c_sig_handler(int sig) {
+#define cbrt(X) _Generic((X), long double \
+ : cbrtl, default \
+ : cbrt)(X)
+ auto char c = '1';
+ extern _Thread_local double _Complex d;
+ static const unsigned long int i = sizeof(float);
+ void f();
+ volatile struct c_struct {
+enum e {};
+union u;
+ };
+ typedef bool boolean;
+label:
+ switch (c) {
+ case ('1'):
+break;
+ default:
+d = 1.2;
+ }
+ goto label;
+ for (; i < 42;) {
+if (d == 1.2)
+ continue;
+else
+ return;
+ }
+ do {
+_Atomic int v = _Alignof(char);
+_Static_assert(42 == 42, "True");
+ } while (c == 42);
+}
+
+void install_c_sig_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, c_sig_handler)) {
+// Handle error
+ }
+}
+
+extern "C" void signal_handler_with_function_pointer(int sig) {
+ void (*funp) (void);
+
[PATCH] D33825: [clang-tidy] signal handler must be plain old function check
bruntib updated this revision to Diff 288617.
bruntib added a comment.
Update lincense.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D33825/new/
https://reviews.llvm.org/D33825
Files:
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
clang-tools-extra/clang-tidy/cert/CMakeLists.txt
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
@@ -0,0 +1,133 @@
+// RUN: %check_clang_tidy %s cert-msc54-cpp %t -- -- -std=c++11
+
+namespace std {
+extern "C" using signalhandler = void(int);
+int signal(int sig, signalhandler *func);
+}
+
+#define SIG_ERR -1
+#define SIGTERM 15
+
+static void sig_handler(int sig) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: signal handlers must be 'extern "C"' [cert-msc54-cpp]
+// CHECK-MESSAGES: :[[@LINE+3]]:39: note: given as a signal parameter here
+
+void install_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, sig_handler))
+return;
+}
+
+extern "C" void cpp_signal_handler(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not use C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: C++ construct used here
+ throw "error message";
+}
+
+void install_cpp_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, cpp_signal_handler))
+return;
+}
+
+void indirect_recursion();
+
+void cpp_like(){
+ try {
+cpp_signal_handler(SIG_ERR);
+ } catch(...) {
+// handle error
+ }
+}
+
+void no_body();
+
+void recursive_function() {
+ indirect_recursion();
+ cpp_like();
+ no_body();
+ recursive_function();
+}
+
+void indirect_recursion() {
+ recursive_function();
+}
+
+extern "C" void signal_handler_with_cpp_function_call(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE-11]]:3: note: function called here
+ // CHECK-MESSAGES: :[[@LINE-23]]:3: note: C++ construct used here
+ recursive_function();
+}
+
+void install_signal_handler_with_cpp_function_call() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_function_call))
+return;
+}
+
+class TestClass {
+public:
+ static void static_function() {}
+};
+
+extern "C" void signal_handler_with_cpp_static_method_call(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: function called here
+ TestClass::static_function();
+}
+
+void install_signal_handler_with_cpp_static_method_call() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_static_method_call))
+return;
+}
+
+
+// Something that does not trigger the check:
+
+extern "C" void c_sig_handler(int sig) {
+#define cbrt(X) _Generic((X), long double \
+ : cbrtl, default \
+ : cbrt)(X)
+ auto char c = '1';
+ extern _Thread_local double _Complex d;
+ static const unsigned long int i = sizeof(float);
+ void f();
+ volatile struct c_struct {
+enum e {};
+union u;
+ };
+ typedef bool boolean;
+label:
+ switch (c) {
+ case ('1'):
+break;
+ default:
+d = 1.2;
+ }
+ goto label;
+ for (; i < 42;) {
+if (d == 1.2)
+ continue;
+else
+ return;
+ }
+ do {
+_Atomic int v = _Alignof(char);
+_Static_assert(42 == 42, "True");
+ } while (c == 42);
+}
+
+void install_c_sig_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, c_sig_handler)) {
+// Handle error
+ }
+}
+
+extern "C" void signal_handler_with_function_pointer(int sig) {
+ void (*funp) (void);
+ funp = _like;
+ funp();
+}
+
+void install_signal_handler_with_function_pointer() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_function_pointer))
+return;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -111,6 +111,7 @@
`cert-mem57-cpp `_,
`cert-msc50-cpp `_,
`cert-msc51-cpp `_,
+ `cert-msc54-cpp `_,
`cert-oop57-cpp `_,
`cert-oop58-cpp `_,
`clang-analyzer-core.DynamicTypePropagation `_,
Index: clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst
[PATCH] D33825: [clang-tidy] signal handler must be plain old function check
bruntib updated this revision to Diff 288606.
bruntib added a comment.
Herald added a subscriber: mgehre.
I rebased the patch so it compiles with master version of LLVM/Clang. I did no
other change, so I would like if this patch would be committed on behalf of
@NorenaLeonetti if the patch is acceptable.
I would kindly ask the reviewers to give some comments if any additional
modification is needed. I run this checker on DuckDB project and this checker
gave two reports on functions which shouldn't be used as signal handler:
duckdb-0.2.0/third_party/sqlsmith/sqlsmith.cc:38:17: do not use C++ constructs
in signal handlers [cert-msc54-cpp]
https://github.com/cwida/duckdb/blob/master/third_party/sqlsmith/sqlsmith.cc#L38
duckdb-0.2.0/tools/shell/shell.c:8828:13: signal handlers must be 'extern "C"'
[cert-msc54-cpp]
https://github.com/cwida/duckdb/blob/master/tools/shell/shell.c#L8828
I haven't found other C++ projects which use signals. However, this checker
reports on the non-compliant code fragments of the corresponding SEI-CERT rule:
https://wiki.sei.cmu.edu/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function.
The two findings above also seem to be true positive.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D33825/new/
https://reviews.llvm.org/D33825
Files:
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
clang-tools-extra/clang-tidy/cert/CMakeLists.txt
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
@@ -0,0 +1,133 @@
+// RUN: %check_clang_tidy %s cert-msc54-cpp %t -- -- -std=c++11
+
+namespace std {
+extern "C" using signalhandler = void(int);
+int signal(int sig, signalhandler *func);
+}
+
+#define SIG_ERR -1
+#define SIGTERM 15
+
+static void sig_handler(int sig) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: signal handlers must be 'extern "C"' [cert-msc54-cpp]
+// CHECK-MESSAGES: :[[@LINE+3]]:39: note: given as a signal parameter here
+
+void install_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, sig_handler))
+return;
+}
+
+extern "C" void cpp_signal_handler(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not use C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: C++ construct used here
+ throw "error message";
+}
+
+void install_cpp_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, cpp_signal_handler))
+return;
+}
+
+void indirect_recursion();
+
+void cpp_like(){
+ try {
+cpp_signal_handler(SIG_ERR);
+ } catch(...) {
+// handle error
+ }
+}
+
+void no_body();
+
+void recursive_function() {
+ indirect_recursion();
+ cpp_like();
+ no_body();
+ recursive_function();
+}
+
+void indirect_recursion() {
+ recursive_function();
+}
+
+extern "C" void signal_handler_with_cpp_function_call(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE-11]]:3: note: function called here
+ // CHECK-MESSAGES: :[[@LINE-23]]:3: note: C++ construct used here
+ recursive_function();
+}
+
+void install_signal_handler_with_cpp_function_call() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_function_call))
+return;
+}
+
+class TestClass {
+public:
+ static void static_function() {}
+};
+
+extern "C" void signal_handler_with_cpp_static_method_call(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: function called here
+ TestClass::static_function();
+}
+
+void install_signal_handler_with_cpp_static_method_call() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_static_method_call))
+return;
+}
+
+
+// Something that does not trigger the check:
+
+extern "C" void c_sig_handler(int sig) {
+#define cbrt(X) _Generic((X), long double \
+ : cbrtl, default \
+ : cbrt)(X)
+ auto char c = '1';
+ extern _Thread_local double _Complex d;
+ static const unsigned long int i = sizeof(float);
+ void f();
+ volatile struct c_struct {
+enum e {};
+union u;
+ };
+ typedef bool boolean;
+label:
+ switch (c) {
+ case ('1'):
+break;
+ default:
+d = 1.2;
+ }
[PATCH] D33825: [clang-tidy] signal handler must be plain old function check
bruntib commandeered this revision. bruntib added a reviewer: NorenaLeonetti. bruntib added a comment. Herald added subscribers: martong, steakhal, jfb. I'll rebase this patch and continue its development. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D33825/new/ https://reviews.llvm.org/D33825 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D80210: [analyzer] Turn off reports in system headers
bruntib added a comment. Hi, Thanks everyone for the investigation in this problem. I find these suggestions reasonable, and I think this commented example is a strong beating card in our hands to convince our users that paths going through system headers are meaningful and important. I'll start implementing this idea and we'll see how much it affects the results in real-world projects. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80210/new/ https://reviews.llvm.org/D80210 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67265: [clang-tidy] Magic number checker shouldn't warn on user defined string literals
bruntib updated this revision to Diff 219097.
bruntib added a comment.
Usually I'm not a big fan of `auto`, but in this case it makes sense not to
duplicate the type name in the same expression. I'll use this rule of thumb in
the future, thanks :)
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67265/new/
https://reviews.llvm.org/D67265
Files:
clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp
Index:
clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp
@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy -std=c++14-or-later %s readability-magic-numbers %t
--
+
+namespace std {
+ class string {};
+ using size_t = decltype(sizeof(int));
+ string operator ""s(const char *, std::size_t);
+ int operator "" s(unsigned long long);
+}
+
+void UserDefinedLiteral() {
+ using std::operator ""s;
+ "Hello World"s;
+ const int i = 3600s;
+ int j = 3600s;
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3600s is a magic number;
consider replacing it with a named constant [readability-magic-numbers]
+}
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -112,10 +112,21 @@
return llvm::any_of(
Result.Context->getParents(ExprResult),
[](const DynTypedNode ) {
-return isUsedToInitializeAConstant(Result, Parent) ||
- // Ignore this instance, because this match reports the location
- // where the template is defined, not where it is instantiated.
- Parent.get();
+if (isUsedToInitializeAConstant(Result, Parent))
+ return true;
+
+// Ignore this instance, because this match reports the location
+// where the template is defined, not where it is instantiated.
+if (Parent.get())
+ return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const auto *UDL = Parent.get())
+ if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
});
}
Index: clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp
@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy -std=c++14-or-later %s readability-magic-numbers %t --
+
+namespace std {
+ class string {};
+ using size_t = decltype(sizeof(int));
+ string operator ""s(const char *, std::size_t);
+ int operator "" s(unsigned long long);
+}
+
+void UserDefinedLiteral() {
+ using std::operator ""s;
+ "Hello World"s;
+ const int i = 3600s;
+ int j = 3600s;
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3600s is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -112,10 +112,21 @@
return llvm::any_of(
Result.Context->getParents(ExprResult),
[](const DynTypedNode ) {
-return isUsedToInitializeAConstant(Result, Parent) ||
- // Ignore this instance, because this match reports the location
- // where the template is defined, not where it is instantiated.
- Parent.get();
+if (isUsedToInitializeAConstant(Result, Parent))
+ return true;
+
+// Ignore this instance, because this match reports the location
+// where the template is defined, not where it is instantiated.
+if (Parent.get())
+ return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const auto *UDL = Parent.get())
+ if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
});
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67265: [clang-tidy] Magic number checker shouldn't warn on user defined string literals
bruntib updated this revision to Diff 219088.
bruntib edited the summary of this revision.
bruntib added a comment.
Thank you for the suggestion. I didn't know about this behavior of
`%check_clang_tidy`.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67265/new/
https://reviews.llvm.org/D67265
Files:
clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp
Index:
clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp
@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy -std=c++14-or-later %s readability-magic-numbers %t
--
+
+namespace std {
+ class string {};
+ using size_t = decltype(sizeof(int));
+ string operator "" s(const char *, std::size_t);
+ int operator "" s(unsigned long long);
+}
+
+void UserDefinedLiteral() {
+ using std::operator ""s;
+ "Hello World"s;
+ const int i = 3600s;
+ int j = 3600s;
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3600s is a magic number;
consider replacing it with a named constant [readability-magic-numbers]
+}
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -112,10 +112,21 @@
return llvm::any_of(
Result.Context->getParents(ExprResult),
[](const DynTypedNode ) {
-return isUsedToInitializeAConstant(Result, Parent) ||
- // Ignore this instance, because this match reports the location
- // where the template is defined, not where it is instantiated.
- Parent.get();
+if (isUsedToInitializeAConstant(Result, Parent))
+ return true;
+
+// Ignore this instance, because this match reports the location
+// where the template is defined, not where it is instantiated.
+if (Parent.get())
+ return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const UserDefinedLiteral *UDL = Parent.get())
+ if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
});
}
Index: clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-magic-numbers-userliteral.cpp
@@ -0,0 +1,16 @@
+// RUN: %check_clang_tidy -std=c++14-or-later %s readability-magic-numbers %t --
+
+namespace std {
+ class string {};
+ using size_t = decltype(sizeof(int));
+ string operator "" s(const char *, std::size_t);
+ int operator "" s(unsigned long long);
+}
+
+void UserDefinedLiteral() {
+ using std::operator ""s;
+ "Hello World"s;
+ const int i = 3600s;
+ int j = 3600s;
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3600s is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -112,10 +112,21 @@
return llvm::any_of(
Result.Context->getParents(ExprResult),
[](const DynTypedNode ) {
-return isUsedToInitializeAConstant(Result, Parent) ||
- // Ignore this instance, because this match reports the location
- // where the template is defined, not where it is instantiated.
- Parent.get();
+if (isUsedToInitializeAConstant(Result, Parent))
+ return true;
+
+// Ignore this instance, because this match reports the location
+// where the template is defined, not where it is instantiated.
+if (Parent.get())
+ return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const UserDefinedLiteral *UDL = Parent.get())
+ if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
});
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D67265: [clang-tidy] Magic number checker shouldn't warn on user defined string literals
bruntib created this revision.
bruntib added reviewers: alexfh, Szelethus, 0x8000-, aaron.ballman.
Herald added subscribers: cfe-commits, xazax.hun, whisperity.
Herald added a project: clang.
The following code snippet results a false positive report in the magic number
checker:
std::string s = "Hello World"s;
The expression "Hello World"s has a StringLiteral in its AST, since this is a
function call on std::string operator "" s(const char* std::size_t) and the
second parameter is passed the length of the string literal.
This patch fixes https://bugs.llvm.org/show_bug.cgi?id=40633
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D67265
Files:
clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
clang-tools-extra/test/clang-tidy/readability-magic-numbers.cpp
Index: clang-tools-extra/test/clang-tidy/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/readability-magic-numbers.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: %check_clang_tidy -std=c++14 %s readability-magic-numbers %t \
// RUN: -config='{CheckOptions: \
// RUN: [{key: readability-magic-numbers.IgnoredIntegerValues, value:
"0;1;2;10;100;"}, \
// RUN: {key: readability-magic-numbers.IgnoredFloatingPointValues, value:
"3.14;2.71828;9.81;1.0;101.0;0x1.2p3"}, \
@@ -197,3 +197,18 @@
return Total;
}
+
+namespace std {
+ class string {};
+ using size_t = decltype(sizeof(int));
+ string operator "" s(const char *, std::size_t);
+ int operator "" s(unsigned long long);
+}
+
+void UserDefinedLiteral() {
+ using std::operator ""s;
+ "Hello World"s;
+ const int i = 3600s;
+ int j = 3600s;
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3600s is a magic number;
consider replacing it with a named constant [readability-magic-numbers]
+}
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -112,10 +112,21 @@
return llvm::any_of(
Result.Context->getParents(ExprResult),
[](const DynTypedNode ) {
-return isUsedToInitializeAConstant(Result, Parent) ||
- // Ignore this instance, because this match reports the location
- // where the template is defined, not where it is instantiated.
- Parent.get();
+if (isUsedToInitializeAConstant(Result, Parent))
+ return true;
+
+// Ignore this instance, because this match reports the location
+// where the template is defined, not where it is instantiated.
+if (Parent.get())
+ return true;
+
+// Don't warn on string user defined literals:
+// std::string s = "Hello World"s;
+if (const UserDefinedLiteral *UDL = Parent.get())
+ if (UDL->getLiteralOperatorKind() == UserDefinedLiteral::LOK_String)
+return true;
+
+return false;
});
}
Index: clang-tools-extra/test/clang-tidy/readability-magic-numbers.cpp
===
--- clang-tools-extra/test/clang-tidy/readability-magic-numbers.cpp
+++ clang-tools-extra/test/clang-tidy/readability-magic-numbers.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: %check_clang_tidy -std=c++14 %s readability-magic-numbers %t \
// RUN: -config='{CheckOptions: \
// RUN: [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
// RUN: {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;1.0;101.0;0x1.2p3"}, \
@@ -197,3 +197,18 @@
return Total;
}
+
+namespace std {
+ class string {};
+ using size_t = decltype(sizeof(int));
+ string operator "" s(const char *, std::size_t);
+ int operator "" s(unsigned long long);
+}
+
+void UserDefinedLiteral() {
+ using std::operator ""s;
+ "Hello World"s;
+ const int i = 3600s;
+ int j = 3600s;
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3600s is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+}
Index: clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
@@ -112,10 +112,21 @@
return llvm::any_of(
Result.Context->getParents(ExprResult),
[](const DynTypedNode ) {
-return isUsedToInitializeAConstant(Result, Parent) ||
- // Ignore this instance, because this match reports the location
- // where the template is defined, not where it is
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
bruntib marked 3 inline comments as done. bruntib added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:202 + SVal l, + unsigned idx = -1u) const; ProgramStateRef CheckLocation(CheckerContext , Szelethus wrote: > I think `Optional` would be nicer. Also, `checkNonNull` as a > function name doesn't scream about why we need an index parameter -- could > you rename it to `IndexOfArg` or something similar? The default value of IdxOfArg comes from my laziness. There were two places where I was not sure what index number should be given, but I added those too. This way no default value or Optional is needed. Omitting this information wouldn't even be reasonable, since there is always an ordinal number of the argument. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1548 - state = checkNonNull(C, state, Dst, DstVal); + state = checkNonNull(C, state, Dst, DstVal, 1); if (!state) NoQ wrote: > You could also pass a description of the parameter (eg., "source" or > "destination"). Could you please give some hint, how to include this in the message? I don't know how to do it concisely. Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:195-196 + llvm::raw_svector_ostream OS(SBuf); + OS << "Null pointer passed as an argument to a 'nonnull' "; + OS << Idx << llvm::getOrdinalSuffix(Idx) << " parameter"; + Szelethus wrote: > whisperity wrote: > > It seems to be as if now you're able to present which parameter is the > > `[[nonnull]]` one. Because of this, the output to the user sounds really > > bad and unfriendly, at least to me. > > > > How about this: > > > > "null pointer passed to nth parameter, but it's marked 'nonnull'"? > > "null pointer passed to nth parameter expecting 'nonnull'"? > > > > Something along these lines. > > > > To a parameter, we're always passing arguments, so saying "as an argument" > > seems redundant. > > > > And because we have the index always in our hands, we don't need to use the > > indefinite article. > Agreed. I used the original message and just extended it with a number. Are you sure that it is a good practice to change a checker message? I'm not against the modification, just a little afraid that somebody may rely on it. It's quite unlikely though, so I changed it :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
bruntib updated this revision to Diff 215754.
bruntib added a comment.
The checker message has been changed according to the suggestion.
The last parameter of checkNonNull() doesn't require a default or optional
value, so it has been fixed.
The parameter has been renamed from Idx to IdxOfArg.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66333/new/
https://reviews.llvm.org/D66333
Files:
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
clang/test/Analysis/misc-ps-region-store.m
clang/test/Analysis/null-deref-ps.c
Index: clang/test/Analysis/null-deref-ps.c
===
--- clang/test/Analysis/null-deref-ps.c
+++ clang/test/Analysis/null-deref-ps.c
@@ -88,21 +88,21 @@
int bar(int* p, int q) __attribute__((nonnull));
int f6(int *p) {
- return !p ? bar(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+ return !p ? bar(p, 1) // expected-warning {{Null pointer passed to 1st parameter expecting 'nonnull'}}
: bar(p, 0); // no-warning
}
int bar2(int* p, int q) __attribute__((nonnull(1)));
int f6b(int *p) {
- return !p ? bar2(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+ return !p ? bar2(p, 1) // expected-warning {{Null pointer passed to 1st parameter expecting 'nonnull'}}
: bar2(p, 0); // no-warning
}
int bar3(int*p, int q, int *r) __attribute__((nonnull(1,3)));
int f6c(int *p, int *q) {
- return !p ? bar3(q, 2, p) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+ return !p ? bar3(q, 2, p) // expected-warning {{Null pointer passed to 3rd parameter expecting 'nonnull'}}
: bar3(p, 2, q); // no-warning
}
Index: clang/test/Analysis/misc-ps-region-store.m
===
--- clang/test/Analysis/misc-ps-region-store.m
+++ clang/test/Analysis/misc-ps-region-store.m
@@ -1205,7 +1205,7 @@
void rdar_8642434_funcB(struct rdar_8642434_typeA *x, struct rdar_8642434_typeA *y) {
rdar_8642434_funcA(x);
if (!y)
-rdar_8642434_funcA(y); // expected-warning{{Null pointer passed as an argument to a 'nonnull' parameter}}
+rdar_8642434_funcA(y); // expected-warning{{Null pointer passed to 1st parameter expecting 'nonnull'}}
}
// - Handle loads and stores from a symbolic index
Index: clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
@@ -6141,12 +6141,12 @@
depth0
extended_message
- Null pointer passed as an argument to a nonnull parameter
+ Null pointer passed to 1st parameter expecting nonnull
message
- Null pointer passed as an argument to a nonnull parameter
+ Null pointer passed to 1st parameter expecting nonnull
- descriptionNull pointer passed as an argument to a nonnull parameter
+ descriptionNull pointer passed to 1st parameter expecting nonnull
categoryAPI
typeArgument with nonnull attribute passed null
check_namecore.NonNullParamChecker
Index: clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
===
--- clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
+++ clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
@@ -10853,12 +10853,12 @@
depth0
extended_message
- Null pointer passed as an argument to a nonnull parameter
+ Null pointer passed to 1st parameter expecting nonnull
message
- Null pointer passed as an argument to a nonnull parameter
+ Null pointer passed to 1st parameter expecting nonnull
- descriptionNull pointer passed as an argument to a nonnull parameter
+ descriptionNull pointer passed to 1st parameter expecting nonnull
categoryAPI
typeArgument with nonnull attribute passed null
check_namecore.NonNullParamChecker
Index: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
@@ -36,7 +36,9 @@
void checkPreCall(const CallEvent , CheckerContext ) const;
std::unique_ptr
- genReportNullAttrNonNull(const ExplodedNode *ErrorN, const Expr *ArgE) const;
+ genReportNullAttrNonNull(const ExplodedNode *ErrorN,
+ const Expr *ArgE,
+ unsigned IdxOfArg) const;
std::unique_ptr
[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings
bruntib added a comment. Thank you for the valuable comments and the review! I'm just planning to register to the bug tracker, because it is getting more relevant due to some other contributions as well. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66333: NonNullParamChecker and CStringChecker parameter number in checker message
bruntib created this revision.
bruntib added reviewers: alexfh, NoQ, Szelethus, xazax.hun.
Herald added subscribers: cfe-commits, rnkovacs.
Herald added a project: clang.
There are some functions which can't be given a null pointer as parameter
either because it has a nonnull attribute or it is declared to have undefined
behavior (e.g. strcmp()). Sometimes it is hard to determine from the checker
message which parameter is null at the invocation, so now this information is
included in the message.
This commit fixes https://bugs.llvm.org/show_bug.cgi?id=39358
Repository:
rC Clang
https://reviews.llvm.org/D66333
Files:
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
clang/test/Analysis/misc-ps-region-store.m
clang/test/Analysis/null-deref-ps.c
Index: clang/test/Analysis/null-deref-ps.c
===
--- clang/test/Analysis/null-deref-ps.c
+++ clang/test/Analysis/null-deref-ps.c
@@ -88,21 +88,21 @@
int bar(int* p, int q) __attribute__((nonnull));
int f6(int *p) {
- return !p ? bar(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+ return !p ? bar(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' 1st parameter}}
: bar(p, 0); // no-warning
}
int bar2(int* p, int q) __attribute__((nonnull(1)));
int f6b(int *p) {
- return !p ? bar2(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+ return !p ? bar2(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' 1st parameter}}
: bar2(p, 0); // no-warning
}
int bar3(int*p, int q, int *r) __attribute__((nonnull(1,3)));
int f6c(int *p, int *q) {
- return !p ? bar3(q, 2, p) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+ return !p ? bar3(q, 2, p) // expected-warning {{Null pointer passed as an argument to a 'nonnull' 3rd parameter}}
: bar3(p, 2, q); // no-warning
}
Index: clang/test/Analysis/misc-ps-region-store.m
===
--- clang/test/Analysis/misc-ps-region-store.m
+++ clang/test/Analysis/misc-ps-region-store.m
@@ -1205,7 +1205,7 @@
void rdar_8642434_funcB(struct rdar_8642434_typeA *x, struct rdar_8642434_typeA *y) {
rdar_8642434_funcA(x);
if (!y)
-rdar_8642434_funcA(y); // expected-warning{{Null pointer passed as an argument to a 'nonnull' parameter}}
+rdar_8642434_funcA(y); // expected-warning{{Null pointer passed as an argument to a 'nonnull' 1st parameter}}
}
// - Handle loads and stores from a symbolic index
Index: clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
@@ -6141,12 +6141,12 @@
depth0
extended_message
- Null pointer passed as an argument to a nonnull parameter
+ Null pointer passed as an argument to a nonnull 1st parameter
message
- Null pointer passed as an argument to a nonnull parameter
+ Null pointer passed as an argument to a nonnull 1st parameter
- descriptionNull pointer passed as an argument to a nonnull parameter
+ descriptionNull pointer passed as an argument to a nonnull 1st parameter
categoryAPI
typeArgument with nonnull attribute passed null
check_namecore.NonNullParamChecker
Index: clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
===
--- clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
+++ clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
@@ -10853,12 +10853,12 @@
depth0
extended_message
- Null pointer passed as an argument to a nonnull parameter
+ Null pointer passed as an argument to a nonnull 1st parameter
message
- Null pointer passed as an argument to a nonnull parameter
+ Null pointer passed as an argument to a nonnull 1st parameter
- descriptionNull pointer passed as an argument to a nonnull parameter
+ descriptionNull pointer passed as an argument to a nonnull 1st parameter
categoryAPI
typeArgument with nonnull attribute passed null
check_namecore.NonNullParamChecker
Index: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
@@ -36,7 +36,9 @@
void checkPreCall(const CallEvent , CheckerContext ) const;
[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings
bruntib updated this revision to Diff 215122.
bruntib added a comment.
Since now the order of diagnostic messages is deterministic, we can count on
this order in the test file.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65065/new/
https://reviews.llvm.org/D65065
Files:
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
Index: clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+ int ex = 42;
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous
temporary values instead [cert-err09-cpp]
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous
temporary values instead [cert-err61-cpp]
+ throw ex;
+}
+
+void doTheJob() {
+ try {
+alwaysThrows();
+ } catch (int&) {
+ }
+}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -742,8 +742,9 @@
const tooling::DiagnosticMessage = LHS.Message;
const tooling::DiagnosticMessage = RHS.Message;
-return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
- std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+return
+ std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+ std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
}
};
struct EqualClangTidyError {
Index: clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+ int ex = 42;
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err09-cpp]
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err61-cpp]
+ throw ex;
+}
+
+void doTheJob() {
+ try {
+alwaysThrows();
+ } catch (int&) {
+ }
+}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -742,8 +742,9 @@
const tooling::DiagnosticMessage = LHS.Message;
const tooling::DiagnosticMessage = RHS.Message;
-return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
- std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+return
+ std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+ std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
}
};
struct EqualClangTidyError {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings
bruntib added a comment. This suggestion would result another strange behavior: if the user disables cert-err09-cpp because he or she doesn't want to see its reports, the other one (cert-err61-cpp) will still report the issue. So he or she has to disable both (or as many aliases it has). As far as I know the idea behind aliases is that a checker can be registered with different options on different names. For example a code style checker can be registered with different names (e.g. GCC-style, LLVM-style, etc.) with different checker options on code styles. This could be another advantage of this patch, since it would be annoying to see a message in which a GCC-style checker warns about an LLVM-style violation. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings
bruntib added a comment. Not exactly. The problem is that it is non-deterministic which checker reports the issue. For example before this patch, in the test file above there was only one report. However, sometimes the report message is: throw expression should throw anonymous temporary values instead [cert-err09-cpp] and sometimes the message is: throw expression should throw anonymous temporary values instead [cert-err61-cpp] (note the content of the square bracket) So after this patch both of these lines will be emitted. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings
bruntib added a comment. Alright, I modified the commit accordingly. Thank you for the suggestions. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings
bruntib updated this revision to Diff 213356.
bruntib edited the summary of this revision.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65065/new/
https://reviews.llvm.org/D65065
Files:
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
Index: clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+ int ex = 42;
+ // At this location both cert-err09-cpp and cert-err61-cpp report the
+ // message. The order of the reports is unknown, that's why the checker name
+ // is not included in the expected warning.
+
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous
temporary values instead
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous
temporary values instead
+ throw ex;
+}
+
+void doTheJob() {
+ try {
+alwaysThrows();
+ } catch (int&) {
+ }
+}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -742,8 +742,9 @@
const tooling::DiagnosticMessage = LHS.Message;
const tooling::DiagnosticMessage = RHS.Message;
-return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
- std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+return
+ std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+ std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
}
};
struct EqualClangTidyError {
Index: clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+ int ex = 42;
+ // At this location both cert-err09-cpp and cert-err61-cpp report the
+ // message. The order of the reports is unknown, that's why the checker name
+ // is not included in the expected warning.
+
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead
+ throw ex;
+}
+
+void doTheJob() {
+ try {
+alwaysThrows();
+ } catch (int&) {
+ }
+}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -742,8 +742,9 @@
const tooling::DiagnosticMessage = LHS.Message;
const tooling::DiagnosticMessage = RHS.Message;
-return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
- std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+return
+ std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+ std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
}
};
struct EqualClangTidyError {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings
bruntib added a comment. Yes, LessClangTidyError would really be the best place for this. The reason of my implementation was that I wanted to bind this feature to a command line option. I don't know if there was any design decision behind the current uniquing logic and I didn't want to break it if there was any. So would you be agree with having duplicate reports by default without any command line flag in case of alias checkers? I believe this would be the most painless solution, since in case of further improvements about including notes, fixes, etc. in the functor, the potential command line option controlling this would be unnecessarily complex. I don't know if there could be any use-case on those. Thanks for the review! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings
bruntib created this revision.
bruntib added reviewers: alexfh, xazax.hun, Szelethus.
Herald added subscribers: cfe-commits, mgrang, rnkovacs, whisperity.
Herald added a project: clang.
In case a checker is registered multiple times as an alias, the emitted
warnings are uniqued by the report message. However, it is random which checker
name is included in the warning. When processing the output of clang-tidy this
behavior caused some problems. So in this commit clang-tidy has been extended
with a --duplicate-reports flag which prevents uniquing the warnings and both
checker's messages are printed under their name.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D65065
Files:
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
clang-tools-extra/clang-tidy/ClangTidyOptions.h
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
clang-tools-extra/test/clang-tidy/duplicate-reports-with.cpp
clang-tools-extra/test/clang-tidy/duplicate-reports-without.cpp
Index: clang-tools-extra/test/clang-tidy/duplicate-reports-without.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports-without.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+ int ex = 42;
+ // It is unknown which checker alias is reporting the warning, that is why
+ // the checker name is not included in the message.
+
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead
+ throw ex;
+}
+
+void doTheJob() {
+ try {
+alwaysThrows();
+ } catch (int&) {
+ }
+}
Index: clang-tools-extra/test/clang-tidy/duplicate-reports-with.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports-with.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t -duplicate-reports --
+
+void alwaysThrows() {
+ int ex = 42;
+ // At this location both cert-err09-cpp and cert-err61-cpp report the
+ // message. The order of the reports is unknown, that's why the checker name
+ // is not included in the expected warning.
+
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead
+ throw ex;
+}
+
+void doTheJob() {
+ try {
+alwaysThrows();
+ } catch (int&) {
+ }
+}
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -78,6 +78,15 @@
cl::init(""),
cl::cat(ClangTidyCategory));
+static cl::opt DuplicateReports("duplicate-reports", cl::desc(R"(
+Some checkers are aliases of others, so the same
+issue may be reported by different checkers at
+the same location. These duplicates are uniqued
+unless this flag is provided.
+)"),
+ cl::init(false),
+ cl::cat(ClangTidyCategory));
+
static cl::opt HeaderFilter("header-filter", cl::desc(R"(
Regular expression matching the names of the
headers to output diagnostics from. Diagnostics
@@ -266,6 +275,7 @@
ClangTidyOptions DefaultOptions;
DefaultOptions.Checks = DefaultChecks;
DefaultOptions.WarningsAsErrors = "";
+ DefaultOptions.DuplicateReports = DuplicateReports;
DefaultOptions.HeaderFilterRegex = HeaderFilter;
DefaultOptions.SystemHeaders = SystemHeaders;
DefaultOptions.FormatStyle = FormatStyle;
@@ -279,6 +289,8 @@
OverrideOptions.Checks = Checks;
if (WarningsAsErrors.getNumOccurrences() > 0)
OverrideOptions.WarningsAsErrors = WarningsAsErrors;
+ if (DuplicateReports.getNumOccurrences() > 0)
+OverrideOptions.DuplicateReports = DuplicateReports;
if (HeaderFilter.getNumOccurrences() > 0)
OverrideOptions.HeaderFilterRegex = HeaderFilter;
if (SystemHeaders.getNumOccurrences() > 0)
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.h
===
--- clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -66,6 +66,10 @@
/// \brief WarningsAsErrors filter.
llvm::Optional WarningsAsErrors;
+ /// \brief Indicates whether warnings emitted by alias checkers should be
+ /// reported multiple times.
+ llvm::Optional DuplicateReports;
+
/// \brief Output warnings from headers matching this filter. Warnings from
/// main files will always be displayed.
llvm::Optional HeaderFilterRegex;
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
[PATCH] D57893: [analyzer] Fix function macro crash
bruntib updated this revision to Diff 190575.
bruntib added a comment.
I've uploaded another version of the last fix. The previous one contained an
UB, although it worked practically.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57893/new/
https://reviews.llvm.org/D57893
Files:
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
test/Analysis/plist-macros-with-expansion.cpp
Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -451,3 +451,21 @@
1 / value; // expected-warning{{Division by zero}}
// expected-warning@-1{{expression result unused}}
}
+
+#define FOO(x) int foo() { return x; }
+#define APPLY_ZERO1(function) function(0)
+
+APPLY_ZERO1(FOO)
+void useZeroApplier1() { (void)(1 / foo()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO1
+// CHECK-NEXT: expansionint foo() { return x; }(0)
+
+#define BAR(x) int bar() { return x; }
+#define APPLY_ZERO2 BAR(0)
+
+APPLY_ZERO2
+void useZeroApplier2() { (void)(1 / bar()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO2
+// CHECK-NEXT: expansionint bar() { return 0; }
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5577,6 +5577,484 @@
+
+ path
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line459
+ col33
+ file0
+
+
+ line459
+ col33
+ file0
+
+
+end
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col39
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line459
+ col37
+ file0
+
+ ranges
+
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Calling foo
+ message
+ Calling foo
+
+
+ kindevent
+ location
+
+ line458
+ col1
+ file0
+
+ depth1
+ extended_message
+ Entered call from useZeroApplier1
+ message
+ Entered call from useZeroApplier1
+
+
+ kindevent
+ location
+
+ line458
+ col1
+ file0
+
+ ranges
+
+
+
+ line458
+ col1
+ file0
+
+
+ line458
+ col16
+ file0
+
+
+
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+
+ line459
+ col37
+ file0
+
+ ranges
+
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Returning from foo
+ message
+ Returning from foo
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col39
+ file0
+
+
+end
+
+
+ line459
+ col35
+ file0
+
+
+ line459
+ col35
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line459
+ col35
+ file0
+
+ ranges
+
+
+
+ line459
+ col33
+ file0
+
+
+ line459
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+
+ macro_expansions
+
+
+ location
+
+ line458
+ col1
+ file0
+
+ nameAPPLY_ZERO1
+ expansionint foo() { return x; }(0)
+
+
+ descriptionDivision by zero
+ categoryLogic error
+ typeDivision by zero
+ check_namecore.DivideZero
+
+ issue_hash_content_of_line_in_context7ff82561a6c752746649d05220deeb40
+ issue_context_kindfunction
+ issue_contextuseZeroApplier1
+ issue_hash_function_offset0
+ location
+
+ line459
+ col35
+ file0
+
+ ExecutedLines
+
+ 0
+
+458
+459
+
+
+
+
+
[PATCH] D57893: [analyzer] Fix function macro crash
bruntib updated this revision to Diff 190485.
bruntib added a comment.
I added a condition before std::next() invocations to check if the next element
is inside the valid interval. This fixes the crash of the build-bot. Sorry for
the ugly bug.
I don't know if there is a more elegant solution.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57893/new/
https://reviews.llvm.org/D57893
Files:
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
test/Analysis/plist-macros-with-expansion.cpp
Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -451,3 +451,21 @@
1 / value; // expected-warning{{Division by zero}}
// expected-warning@-1{{expression result unused}}
}
+
+#define FOO(x) int foo() { return x; }
+#define APPLY_ZERO1(function) function(0)
+
+APPLY_ZERO1(FOO)
+void useZeroApplier1() { (void)(1 / foo()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO1
+// CHECK-NEXT: expansionint foo() { return x; }(0)
+
+#define BAR(x) int bar() { return x; }
+#define APPLY_ZERO2 BAR(0)
+
+APPLY_ZERO2
+void useZeroApplier2() { (void)(1 / bar()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO2
+// CHECK-NEXT: expansionint bar() { return 0; }
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5577,6 +5577,484 @@
+
+ path
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line459
+ col33
+ file0
+
+
+ line459
+ col33
+ file0
+
+
+end
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col39
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line459
+ col37
+ file0
+
+ ranges
+
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Calling foo
+ message
+ Calling foo
+
+
+ kindevent
+ location
+
+ line458
+ col1
+ file0
+
+ depth1
+ extended_message
+ Entered call from useZeroApplier1
+ message
+ Entered call from useZeroApplier1
+
+
+ kindevent
+ location
+
+ line458
+ col1
+ file0
+
+ ranges
+
+
+
+ line458
+ col1
+ file0
+
+
+ line458
+ col16
+ file0
+
+
+
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+
+ line459
+ col37
+ file0
+
+ ranges
+
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Returning from foo
+ message
+ Returning from foo
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col39
+ file0
+
+
+end
+
+
+ line459
+ col35
+ file0
+
+
+ line459
+ col35
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line459
+ col35
+ file0
+
+ ranges
+
+
+
+ line459
+ col33
+ file0
+
+
+ line459
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+
+ macro_expansions
+
+
+ location
+
+ line458
+ col1
+ file0
+
+ nameAPPLY_ZERO1
+ expansionint foo() { return x; }(0)
+
+
+ descriptionDivision by zero
+ categoryLogic error
+ typeDivision by zero
+ check_namecore.DivideZero
+
+ issue_hash_content_of_line_in_context7ff82561a6c752746649d05220deeb40
+ issue_context_kindfunction
+ issue_contextuseZeroApplier1
+ issue_hash_function_offset0
+ location
[PATCH] D57893: [analyzer] Fix function macro crash
bruntib updated this revision to Diff 190225.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57893/new/
https://reviews.llvm.org/D57893
Files:
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
test/Analysis/plist-macros-with-expansion.cpp
Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -451,3 +451,21 @@
1 / value; // expected-warning{{Division by zero}}
// expected-warning@-1{{expression result unused}}
}
+
+#define FOO(x) int foo() { return x; }
+#define APPLY_ZERO1(function) function(0)
+
+APPLY_ZERO1(FOO)
+void useZeroApplier1() { (void)(1 / foo()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO1
+// CHECK-NEXT: expansionint foo() { return x; }(0)
+
+#define BAR(x) int bar() { return x; }
+#define APPLY_ZERO2 BAR(0)
+
+APPLY_ZERO2
+void useZeroApplier2() { (void)(1 / bar()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO2
+// CHECK-NEXT: expansionint bar() { return 0; }
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5577,6 +5577,484 @@
+
+ path
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line459
+ col33
+ file0
+
+
+ line459
+ col33
+ file0
+
+
+end
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col39
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line459
+ col37
+ file0
+
+ ranges
+
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Calling foo
+ message
+ Calling foo
+
+
+ kindevent
+ location
+
+ line458
+ col1
+ file0
+
+ depth1
+ extended_message
+ Entered call from useZeroApplier1
+ message
+ Entered call from useZeroApplier1
+
+
+ kindevent
+ location
+
+ line458
+ col1
+ file0
+
+ ranges
+
+
+
+ line458
+ col1
+ file0
+
+
+ line458
+ col16
+ file0
+
+
+
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+
+ line459
+ col37
+ file0
+
+ ranges
+
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Returning from foo
+ message
+ Returning from foo
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line459
+ col37
+ file0
+
+
+ line459
+ col39
+ file0
+
+
+end
+
+
+ line459
+ col35
+ file0
+
+
+ line459
+ col35
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line459
+ col35
+ file0
+
+ ranges
+
+
+
+ line459
+ col33
+ file0
+
+
+ line459
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+
+ macro_expansions
+
+
+ location
+
+ line458
+ col1
+ file0
+
+ nameAPPLY_ZERO1
+ expansionint foo() { return x; }(0)
+
+
+ descriptionDivision by zero
+ categoryLogic error
+ typeDivision by zero
+ check_namecore.DivideZero
+
+ issue_hash_content_of_line_in_context7ff82561a6c752746649d05220deeb40
+ issue_context_kindfunction
+ issue_contextuseZeroApplier1
+ issue_hash_function_offset0
+ location
+
+ line459
+ col35
+ file0
+
+ ExecutedLines
+
+ 0
+
+458
+459
+
+
+
+
+ path
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line468
+ col33
+
[PATCH] D57893: [analyzer] Fix function macro crash
bruntib added a comment. I rebased the patch on the current master. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57893/new/ https://reviews.llvm.org/D57893 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57891: [analyzer] Fix infinite recursion in printing macros
bruntib updated this revision to Diff 189670.
bruntib added a comment.
Herald added subscribers: Charusso, jdoerfert, whisperity.
I added a test case for this recursive case. There is also a TODO in the code
indicating the place where an additional fix will be required.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57891/new/
https://reviews.llvm.org/D57891
Files:
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
test/Analysis/plist-macros-with-expansion.cpp
Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -440,3 +440,14 @@
}
// CHECK: nameYET_ANOTHER_SET_TO_NULL
// CHECK-NEXT: expansionprint((void *)5); print((void *)Remember the Vasa); ptr = nullptr;
+
+int garbage_value;
+
+#define REC_MACRO_FUNC(REC_MACRO_PARAM) garbage_##REC_MACRO_PARAM
+#define value REC_MACRO_FUNC(value)
+
+void recursiveMacroUser() {
+ if (value == 0)
+1 / value; // expected-warning{{Division by zero}}
+ // expected-warning@-1{{expression result unused}}
+}
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5443,6 +5443,140 @@
+
+ path
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line450
+ col3
+ file0
+
+
+ line450
+ col4
+ file0
+
+
+end
+
+
+ line450
+ col7
+ file0
+
+
+ line450
+ col11
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line450
+ col7
+ file0
+
+ ranges
+
+
+
+ line450
+ col7
+ file0
+
+
+ line450
+ col16
+ file0
+
+
+
+ depth0
+ extended_message
+ Assuming garbage_value is equal to 0
+ message
+ Assuming garbage_value is equal to 0
+
+
+ kindevent
+ location
+
+ line451
+ col7
+ file0
+
+ ranges
+
+
+
+ line451
+ col5
+ file0
+
+
+ line451
+ col13
+ file0
+
+
+
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+
+ macro_expansions
+
+
+ location
+
+ line450
+ col7
+ file0
+
+ namevalue
+ expansiongarbage_
+
+
+ descriptionDivision by zero
+ categoryLogic error
+ typeDivision by zero
+ check_namecore.DivideZero
+
+ issue_hash_content_of_line_in_context1f3c94860e67b6b863e956bd67e49f1d
+ issue_context_kindfunction
+ issue_contextrecursiveMacroUser
+ issue_hash_function_offset2
+ location
+
+ line451
+ col7
+ file0
+
+ ExecutedLines
+
+ 0
+
+449
+450
+451
+
+
+
files
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -22,6 +22,7 @@
#include "clang/StaticAnalyzer/Core/IssueHash.h"
#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
#include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Casting.h"
@@ -776,10 +777,20 @@
/// As we expand the last line, we'll immediately replace PRINT(str) with
/// print(x). The information that both 'str' and 'x' refers to the same string
/// is an information we have to forward, hence the argument \p PrevArgs.
-static std::string getMacroNameAndPrintExpansion(TokenPrinter ,
- SourceLocation MacroLoc,
- const Preprocessor ,
- const MacroArgMap );
+///
+/// To avoid infinite recursion we maintain the already processed tokens in
+/// a set. This is carried as a parameter through the recursive calls. The set
+/// is extended with the currently processed token and after processing it, the
+/// token is removed. If the token is already in the set, then recursion stops:
+///
+/// #define f(y) x
+/// #define x f(x)
+static std::string getMacroNameAndPrintExpansion(
+TokenPrinter ,
+SourceLocation MacroLoc,
+const Preprocessor ,
+const
[PATCH] D57893: [analyzer] Fix function macro crash
bruntib updated this revision to Diff 186161.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57893/new/
https://reviews.llvm.org/D57893
Files:
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
test/Analysis/plist-macros-with-expansion.cpp
Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -441,3 +441,21 @@
}
// CHECK: nameYET_ANOTHER_SET_TO_NULL
// CHECK-NEXT: expansionprint((void *)5); print((void *)Remember the Vasa); ptr = nullptr;
+
+#define FOO(x) int foo() { return x; }
+#define APPLY_ZERO1(function) function(0)
+
+APPLY_ZERO1(FOO)
+void useZeroApplier1() { (void)(1 / foo()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO1
+// CHECK-NEXT: expansionint foo() { return x; }(0)
+
+#define BAR(x) int bar() { return x; }
+#define APPLY_ZERO2 BAR(0)
+
+APPLY_ZERO2
+void useZeroApplier2() { (void)(1 / bar()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO2
+// CHECK-NEXT: expansionint bar() { return 0; }
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5168,6 +5168,468 @@
file0
+
+ path
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line449
+ col33
+ file0
+
+
+ line449
+ col33
+ file0
+
+
+end
+
+
+ line449
+ col37
+ file0
+
+
+ line449
+ col39
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line449
+ col37
+ file0
+
+ ranges
+
+
+
+ line449
+ col37
+ file0
+
+
+ line449
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Calling foo
+ message
+ Calling foo
+
+
+ kindevent
+ location
+
+ line448
+ col1
+ file0
+
+ depth1
+ extended_message
+ Entered call from useZeroApplier1
+ message
+ Entered call from useZeroApplier1
+
+
+ kindevent
+ location
+
+ line448
+ col1
+ file0
+
+ ranges
+
+
+
+ line448
+ col1
+ file0
+
+
+ line448
+ col16
+ file0
+
+
+
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+
+ line449
+ col37
+ file0
+
+ ranges
+
+
+
+ line449
+ col37
+ file0
+
+
+ line449
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Returning from foo
+ message
+ Returning from foo
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line449
+ col37
+ file0
+
+
+ line449
+ col39
+ file0
+
+
+end
+
+
+ line449
+ col35
+ file0
+
+
+ line449
+ col35
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line449
+ col35
+ file0
+
+ ranges
+
+
+
+ line449
+ col33
+ file0
+
+
+ line449
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+
+ macro_expansions
+
+
+ location
+
+ line448
+ col1
+ file0
+
+ nameAPPLY_ZERO1
+ expansionint foo() { return x; }(0)
+
+
+ descriptionDivision by zero
+ categoryLogic error
+ typeDivision by zero
+ check_namecore.DivideZero
+
+ issue_hash_content_of_line_in_context7ff82561a6c752746649d05220deeb40
+ issue_context_kindfunction
+ issue_contextuseZeroApplier1
+ issue_hash_function_offset0
+ location
+
+ line449
+ col35
+ file0
+
+
+
+ path
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line458
+ col33
+ file0
+
+
+ line458
+
[PATCH] D57893: [analyzer] Fix function macro crash
bruntib updated this revision to Diff 185971.
bruntib added a comment.
There was another place where this crash could have happened.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57893/new/
https://reviews.llvm.org/D57893
Files:
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
test/Analysis/plist-macros-with-expansion.cpp
Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -441,3 +441,17 @@
}
// CHECK: nameYET_ANOTHER_SET_TO_NULL
// CHECK-NEXT: expansionprint((void *)5); print((void *)Remember the Vasa); ptr = nullptr;
+
+#define FOO(x) int foo() { return x; }
+#define APPLY_ZERO1(function) function(0)
+#define BAR(x) int bar() { return x; }
+#define APPLY_ZERO2 BAR(0)
+APPLY_ZERO1(FOO)
+APPLY_ZERO2
+void useZeroApplier1() { (void)(1 / foo()); } // expected-warning{{Division by zero}}
+void useZeroApplier2() { (void)(1 / bar()); } // expected-warning{{Division by zero}}
+
+// CHECK: nameAPPLY_ZERO1
+// CHECK-NEXT: expansionint foo() { return x; }(0)
+// CHECK: nameAPPLY_ZERO2
+// CHECK-NEXT: expansionint bar() { return 0; }
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5168,6 +5168,468 @@
file0
+
+ path
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line451
+ col33
+ file0
+
+
+ line451
+ col33
+ file0
+
+
+end
+
+
+ line451
+ col37
+ file0
+
+
+ line451
+ col39
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line451
+ col37
+ file0
+
+ ranges
+
+
+
+ line451
+ col37
+ file0
+
+
+ line451
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Calling foo
+ message
+ Calling foo
+
+
+ kindevent
+ location
+
+ line449
+ col1
+ file0
+
+ depth1
+ extended_message
+ Entered call from useZeroApplier1
+ message
+ Entered call from useZeroApplier1
+
+
+ kindevent
+ location
+
+ line449
+ col1
+ file0
+
+ ranges
+
+
+
+ line449
+ col1
+ file0
+
+
+ line449
+ col16
+ file0
+
+
+
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+
+ line451
+ col37
+ file0
+
+ ranges
+
+
+
+ line451
+ col37
+ file0
+
+
+ line451
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Returning from foo
+ message
+ Returning from foo
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line451
+ col37
+ file0
+
+
+ line451
+ col39
+ file0
+
+
+end
+
+
+ line451
+ col35
+ file0
+
+
+ line451
+ col35
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line451
+ col35
+ file0
+
+ ranges
+
+
+
+ line451
+ col33
+ file0
+
+
+ line451
+ col41
+ file0
+
+
+
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+
+ macro_expansions
+
+
+ location
+
+ line449
+ col1
+ file0
+
+ nameAPPLY_ZERO1
+ expansionint foo() { return x; }(0)
+
+
+ descriptionDivision by zero
+ categoryLogic error
+ typeDivision by zero
+ check_namecore.DivideZero
+
+ issue_hash_content_of_line_in_context7ff82561a6c752746649d05220deeb40
+ issue_context_kindfunction
+ issue_contextuseZeroApplier1
+ issue_hash_function_offset0
+ location
+
+ line451
+ col35
+ file0
+
+
+
+ path
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line452
+
[PATCH] D57893: [analyzer] Fix function macro crash
bruntib updated this revision to Diff 185957.
bruntib added a comment.
I've added a test case.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57893/new/
https://reviews.llvm.org/D57893
Files:
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
test/Analysis/plist-macros-with-expansion.cpp
Index: test/Analysis/plist-macros-with-expansion.cpp
===
--- test/Analysis/plist-macros-with-expansion.cpp
+++ test/Analysis/plist-macros-with-expansion.cpp
@@ -441,3 +441,13 @@
}
// CHECK: nameYET_ANOTHER_SET_TO_NULL
// CHECK-NEXT: expansionprint((void *)5); print((void *)Remember the Vasa); ptr = nullptr;
+
+#define FOO(x) int bar() { return x; }
+#define APPLY_ZERO(function) function(0)
+APPLY_ZERO(FOO)
+void useZeroApplier() {
+ (void)(1 / bar()); // expected-warning{{Division by zero}}
+}
+
+// CHECK: nameAPPLY_ZERO
+// CHECK-NEXT: expansionint bar() { return x; }(0)
Index: test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -5168,6 +5168,237 @@
file0
+
+ path
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line449
+ col10
+ file0
+
+
+ line449
+ col10
+ file0
+
+
+end
+
+
+ line449
+ col14
+ file0
+
+
+ line449
+ col16
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line449
+ col14
+ file0
+
+ ranges
+
+
+
+ line449
+ col14
+ file0
+
+
+ line449
+ col18
+ file0
+
+
+
+ depth0
+ extended_message
+ Calling bar
+ message
+ Calling bar
+
+
+ kindevent
+ location
+
+ line447
+ col1
+ file0
+
+ depth1
+ extended_message
+ Entered call from useZeroApplier
+ message
+ Entered call from useZeroApplier
+
+
+ kindevent
+ location
+
+ line447
+ col1
+ file0
+
+ ranges
+
+
+
+ line447
+ col1
+ file0
+
+
+ line447
+ col15
+ file0
+
+
+
+ depth1
+ extended_message
+ Returning zero
+ message
+ Returning zero
+
+
+ kindevent
+ location
+
+ line449
+ col14
+ file0
+
+ ranges
+
+
+
+ line449
+ col14
+ file0
+
+
+ line449
+ col18
+ file0
+
+
+
+ depth0
+ extended_message
+ Returning from bar
+ message
+ Returning from bar
+
+
+ kindcontrol
+ edges
+
+
+start
+
+
+ line449
+ col14
+ file0
+
+
+ line449
+ col16
+ file0
+
+
+end
+
+
+ line449
+ col12
+ file0
+
+
+ line449
+ col12
+ file0
+
+
+
+
+
+
+ kindevent
+ location
+
+ line449
+ col12
+ file0
+
+ ranges
+
+
+
+ line449
+ col10
+ file0
+
+
+ line449
+ col18
+ file0
+
+
+
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+
+ macro_expansions
+
+
+ location
+
+ line447
+ col1
+ file0
+
+ nameAPPLY_ZERO
+ expansionint bar() { return x; }(0)
+
+
+ descriptionDivision by zero
+ categoryLogic error
+ typeDivision by zero
+ check_namecore.DivideZero
+
+ issue_hash_content_of_line_in_contextb41a3835d64fddaac63749c968f17e81
+ issue_context_kindfunction
+ issue_contextuseZeroApplier
+ issue_hash_function_offset1
+ location
+
+ line449
+ col12
+ file0
+
+
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -879,8 +879,18 @@
getMacroNameAndPrintExpansion(Printer, ArgIt->getLocation(), PP,
Info.Args, AlreadyProcessedTokens);
-if
[PATCH] D57893: [analyzer] Fix function macro crash
bruntib created this revision.
bruntib added reviewers: NoQ, george.karpenkov, Szelethus, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, rnkovacs, szepet, baloghadamsoftware.
Herald added a project: clang.
When there is a functor-like macro which is passed as parameter to another
"function" macro then its parameters are not listed at the place of expansion:
#define foo(x) int bar() { return x; }
#define hello(fvar) fvar(0)
hello(foo)
int main() { 1 / bar(); }
Expansion of hello(foo) asserted Clang, because it expected an l_paren token in
the 3rd line after "foo", since it is a function-like token.
Repository:
rC Clang
https://reviews.llvm.org/D57893
Files:
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -879,8 +879,18 @@
getMacroNameAndPrintExpansion(Printer, ArgIt->getLocation(), PP,
Info.Args, AlreadyProcessedTokens);
-if (MI->getNumParams() != 0)
- ArgIt = getMatchingRParen(++ArgIt, ArgEnd);
+// Peek the next token if it is a tok::l_paren. This way we can decide
+// if this is the application or just a reference to a function maxro
+// symbol:
+//
+// #define apply(f) ...
+// #define func(x) ...
+// apply(func)
+// apply(func(42))
+if ((++ArgIt)->is(tok::l_paren))
+ ArgIt = getMatchingRParen(ArgIt, ArgEnd);
+else
+ --ArgIt;
}
continue;
}
@@ -941,8 +951,16 @@
return { MacroName, MI, {} };
RawLexer.LexFromRawLexer(TheTok);
- assert(TheTok.is(tok::l_paren) &&
- "The token after the macro's identifier token should be '('!");
+ // When this is a token which expands to another macro function then its
+ // parentheses are not at its expansion locaiton. For example:
+ //
+ // #define foo(x) int bar() { return x; }
+ // #define apply_zero(f) f(0)
+ // apply_zero(foo)
+ // ^
+ // This is not a tok::l_paren, but foo is a function.
+ if (TheTok.isNot(tok::l_paren))
+return { MacroName, MI, {} };
MacroArgMap Args;
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -879,8 +879,18 @@
getMacroNameAndPrintExpansion(Printer, ArgIt->getLocation(), PP,
Info.Args, AlreadyProcessedTokens);
-if (MI->getNumParams() != 0)
- ArgIt = getMatchingRParen(++ArgIt, ArgEnd);
+// Peek the next token if it is a tok::l_paren. This way we can decide
+// if this is the application or just a reference to a function maxro
+// symbol:
+//
+// #define apply(f) ...
+// #define func(x) ...
+// apply(func)
+// apply(func(42))
+if ((++ArgIt)->is(tok::l_paren))
+ ArgIt = getMatchingRParen(ArgIt, ArgEnd);
+else
+ --ArgIt;
}
continue;
}
@@ -941,8 +951,16 @@
return { MacroName, MI, {} };
RawLexer.LexFromRawLexer(TheTok);
- assert(TheTok.is(tok::l_paren) &&
- "The token after the macro's identifier token should be '('!");
+ // When this is a token which expands to another macro function then its
+ // parentheses are not at its expansion locaiton. For example:
+ //
+ // #define foo(x) int bar() { return x; }
+ // #define apply_zero(f) f(0)
+ // apply_zero(foo)
+ // ^
+ // This is not a tok::l_paren, but foo is a function.
+ if (TheTok.isNot(tok::l_paren))
+return { MacroName, MI, {} };
MacroArgMap Args;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57892: [analyzer] Fix macro printer crash when macro comes from another translation unit
bruntib created this revision.
bruntib added reviewers: NoQ, george.karpenkov, Szelethus, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, rnkovacs, szepet, baloghadamsoftware.
Herald added a project: clang.
Repository:
rC Clang
https://reviews.llvm.org/D57892
Files:
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -820,6 +820,9 @@
return Info.Name;
AlreadyProcessedTokens.insert(IDInfo);
+ if (!Info.MI)
+return Info.Name;
+
// Manually expand its arguments from the previous macro.
Info.Args.expandFromPrevMacro(PrevArgs);
@@ -917,7 +920,14 @@
assert(II && "Failed to acquire the IndetifierInfo for the macro!");
const MacroInfo *MI = getMacroInfoForLocation(PP, SM, II, ExpanLoc);
- assert(MI && "The macro must've been defined at it's expansion location!");
+ // assert(MI && "The macro must've been defined at it's expansion
location!");
+ //
+ // We should always be able to obtain the MacroInfo in a given TU, but if
+ // we're running the analyzer with CTU, the Preprocessor won't contain the
+ // directive history (or anything for that matter) from another TU.
+ // TODO: assert when we're not running with CTU.
+ if (!MI)
+return { MacroName, MI, {} };
// Acquire the macro's arguments.
//
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -820,6 +820,9 @@
return Info.Name;
AlreadyProcessedTokens.insert(IDInfo);
+ if (!Info.MI)
+return Info.Name;
+
// Manually expand its arguments from the previous macro.
Info.Args.expandFromPrevMacro(PrevArgs);
@@ -917,7 +920,14 @@
assert(II && "Failed to acquire the IndetifierInfo for the macro!");
const MacroInfo *MI = getMacroInfoForLocation(PP, SM, II, ExpanLoc);
- assert(MI && "The macro must've been defined at it's expansion location!");
+ // assert(MI && "The macro must've been defined at it's expansion location!");
+ //
+ // We should always be able to obtain the MacroInfo in a given TU, but if
+ // we're running the analyzer with CTU, the Preprocessor won't contain the
+ // directive history (or anything for that matter) from another TU.
+ // TODO: assert when we're not running with CTU.
+ if (!MI)
+return { MacroName, MI, {} };
// Acquire the macro's arguments.
//
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57891: [analyzer] Fix infinite recursion in printing macros
bruntib created this revision.
bruntib added reviewers: NoQ, george.karpenkov, Szelethus, xazax.hun.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, rnkovacs, szepet, baloghadamsoftware.
Herald added a project: clang.
#define f(y) x
#define x f(x)
int main() { x; }
This example results a compilation error since "x" in the first line was not
defined earlier. However, the macro expression printer goes to an infinite
recursion on this example.
Repository:
rC Clang
https://reviews.llvm.org/D57891
Files:
lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -23,6 +23,7 @@
#include "clang/StaticAnalyzer/Core/IssueHash.h"
#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
#include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Casting.h"
@@ -734,10 +735,20 @@
/// As we expand the last line, we'll immediately replace PRINT(str) with
/// print(x). The information that both 'str' and 'x' refers to the same string
/// is an information we have to forward, hence the argument \p PrevArgs.
-static std::string getMacroNameAndPrintExpansion(TokenPrinter ,
- SourceLocation MacroLoc,
- const Preprocessor ,
- const MacroArgMap );
+///
+/// To avoid infinite recursion we maintain the already processed tokens in
+/// a set. This is carried as a parameter through the recursive calls. The set
+/// is extended with the currently processed token and after processing it, the
+/// token is removed. If the token is already in the set, then recursion stops:
+///
+/// #define f(y) x
+/// #define x f(x)
+static std::string getMacroNameAndPrintExpansion(
+TokenPrinter ,
+SourceLocation MacroLoc,
+const Preprocessor ,
+const MacroArgMap ,
+llvm::SmallPtrSet );
/// Retrieves the name of the macro and what it's arguments expand into
/// at \p ExpanLoc.
@@ -786,19 +797,28 @@
llvm::SmallString<200> ExpansionBuf;
llvm::raw_svector_ostream OS(ExpansionBuf);
TokenPrinter Printer(OS, PP);
+ llvm::SmallPtrSet AlreadyProcessedTokens;
- return { getMacroNameAndPrintExpansion(Printer, MacroLoc, PP, MacroArgMap{}),
+ return { getMacroNameAndPrintExpansion(Printer, MacroLoc, PP, MacroArgMap{},
+ AlreadyProcessedTokens),
OS.str() };
}
-static std::string getMacroNameAndPrintExpansion(TokenPrinter ,
- SourceLocation MacroLoc,
- const Preprocessor ,
- const MacroArgMap ) {
+static std::string getMacroNameAndPrintExpansion(
+TokenPrinter ,
+SourceLocation MacroLoc,
+const Preprocessor ,
+const MacroArgMap ,
+llvm::SmallPtrSet ) {
const SourceManager = PP.getSourceManager();
MacroNameAndArgs Info = getMacroNameAndArgs(SM.getExpansionLoc(MacroLoc),
PP);
+ IdentifierInfo* IDInfo = PP.getIdentifierInfo(Info.Name);
+
+ if (AlreadyProcessedTokens.find(IDInfo) != AlreadyProcessedTokens.end())
+return Info.Name;
+ AlreadyProcessedTokens.insert(IDInfo);
// Manually expand its arguments from the previous macro.
Info.Args.expandFromPrevMacro(PrevArgs);
@@ -822,7 +842,8 @@
// macro.
if (const MacroInfo *MI =
getMacroInfoForLocation(PP, SM, II, T.getLocation()))
{
- getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP, Info.Args);
+ getMacroNameAndPrintExpansion(Printer, T.getLocation(), PP, Info.Args,
+AlreadyProcessedTokens);
// If this is a function-like macro, skip its arguments, as
// getExpandedMacro() already printed them. If this is the case, let's
@@ -854,7 +875,7 @@
}
getMacroNameAndPrintExpansion(Printer, ArgIt->getLocation(), PP,
- Info.Args);
+ Info.Args, AlreadyProcessedTokens);
if (MI->getNumParams() != 0)
ArgIt = getMatchingRParen(++ArgIt, ArgEnd);
}
@@ -866,6 +887,8 @@
Printer.printToken(T);
}
+ AlreadyProcessedTokens.erase(IDInfo);
+
return Info.Name;
}
Index: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -23,6 +23,7 @@
#include "clang/StaticAnalyzer/Core/IssueHash.h"
#include "clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h"
#include "llvm/ADT/Statistic.h"
+#include
[PATCH] D57890: [analyzer] Fix in self assignment checker
bruntib created this revision.
bruntib added reviewers: NoQ, george.karpenkov, Szelethus, xazax.hun,
baloghadamsoftware.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, rnkovacs, szepet.
Herald added a project: clang.
For self assignment checker it was necessary to force checking of assignment
operators even if those are not called. The reason of this is to check whether
"this" is equal to the address of the assignee object.
The buffer overlap checker checks if the intervals of the arguments of a
memcpy() call are disjoint. If a class has an array member then the compiler
generated assignment operator copies it with memcpy() function without checking
self assignment at the beginning. Since the analyzer forces the check of
assignment operators, the buffer overflow checker reported a false positive on
classes with compiler generated assignment operator and array member.
This commit prevents the forced check of compiler generated assignment
operators.
Repository:
rC Clang
https://reviews.llvm.org/D57890
Files:
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -450,7 +450,7 @@
// where it may not. (cplusplus.SelfAssignmentChecker)
if (const auto *MD = dyn_cast(D)) {
if (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator())
- return false;
+ return !MD->isUserProvided();
}
// Otherwise, if we visited the function before, do not reanalyze it.
Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -450,7 +450,7 @@
// where it may not. (cplusplus.SelfAssignmentChecker)
if (const auto *MD = dyn_cast(D)) {
if (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator())
- return false;
+ return !MD->isUserProvided();
}
// Otherwise, if we visited the function before, do not reanalyze it.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
