[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D33825#2246369 , @jfb wrote:

> MSC54-CPP refers to POF, which as I pointed out above isn't relevant anymore. 
> I'd much rather have a diagnostic which honors the state of things after 
> http://wg21.link/p0270.

I agree, and I've commented on that rule to remind the folks at CERT that the 
rule has largely gone stale.

> Additionally, lots of what MSC54-CPP intends is implementation-defined. We 
> shouldn't diagnose for things that clang has defined as signal safe, at least 
> not by default.

Also agreed.




Comment at: 
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:42
+
+const char *cxxStmtText(const Stmt *stmt) {
+  if (llvm::isa(stmt))

I'm not super keen on this approach because as new C++ constructs are added, 
this will continually need to be updated, which is a maintenance burden I think 
we should try to avoid. I would rather use some generic wording or tie the 
wording automatically to something provided by the `Stmt` class.



Comment at: 
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:44
+  if (llvm::isa(stmt))
+return "Binding temporary C++ expression here";
+  else if (llvm::isa(stmt))

Diagnostics in clang-tidy should start with a lowercase letter



Comment at: 
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:91
+const char *cxxDeclText(const Decl *decl) {
+  if (llvm::isa(decl))
+return "Constructor declared here";

Similar concerns here.


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

[JF Bastien via Phabricator via cfe-commits]

jfb requested changes to this revision.
jfb added a comment.
This revision now requires changes to proceed.

MSC54-CPP refers to POF, which as I pointed out above isn't relevant anymore. 
I'd much rather have a diagnostic which honors the state of things after 
http://wg21.link/p0270.

Additionally, lots of what MSC54-CPP intends is implementation-defined. We 
shouldn't diagnose for things that clang has defined as signal safe, at least 
not by default.

From the paper, I'd like to see tests for these:

- a call to any standard library function, except for plain lock-free atomic 
atomic operations and functions explicitly identified as signal-safe. [Note: 
This implicitly excludes the use of new and delete expressions that rely on a 
library-provided memory allocator. – end note]; -- here I'd like to explicitly 
see the list of signal-safe functions, and examples of ones that are not
- an access to an object with thread storage duration;
- a dynamic_cast expression;
- throwing of an exception;
- control entering a try-block or function-try-block; or
- initialization of a variable with static storage duration requiring dynamic 
initialization (3.6.3 [basic.start.dynamic], 6.7 [stmt.decl])). [ Note: Such 
initialization might occur because it is the first odr-use (3.2 
[basic.def.odr]) of that variable. -- end note ]
- waiting for the completion of the initialization of a variable with static 
storage duration (6.7 [stmt.decl]).




Comment at: 
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:84
+  else if (llvm::isa(stmt))
+return "Construction of an unresolved type here";
+  else

Most of the above are fine per p0270, and most don't have a test at the moment. 



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst:15
+static void sig_handler(int sig) {}
+// warning: use 'external C' prefix for signal handlers
+

'extern', not 'external'



Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst:23
+extern "C" void cpp_signal_handler(int sig) {
+  // warning: do not use C++ constructs in signal handlers
+  throw "error message";

Update the example too.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:74
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: note: function called here
+  // CHECK-MESSAGES: TestClass::static_function();
+  TestClass::static_function();

I don't think this should diagnose, it is signal safe for clang's 
implementation, and allowed by p0270.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:91
+  auto char c = '1';
+  extern _Thread_local double _Complex d;
+  static const unsigned long int i = sizeof(float);

_Thread_local isn't signal safe per p0270.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:114
+  do {
+_Atomic int v = _Alignof(char);
+_Static_assert(42 == 42, "True");

The atomic isn't used here, and atomic initialization isn't atomic, so the test 
isn't sufficient.


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

[Tibor Brunner via Phabricator via cfe-commits]

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

[Tibor Brunner via Phabricator via cfe-commits]

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

[Tibor Brunner via Phabricator via cfe-commits]

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

[JF Bastien via Phabricator via cfe-commits]

jfb requested changes to this revision.
jfb added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: dexonsmith.

Please consider these changes, and whether this is relevant as implemented: 
http://wg21.link/p0270




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";

"C++ construct" isn't particularly useful. Here it needs to call out throwing 
exceptions.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:57
+  // CHECK-MESSAGES: :[[@LINE-11]]:3: note: function called here
+  // CHECK-MESSAGES: :[[@LINE-23]]:3: note: C++ construct used here
+  recursive_function();

Here too, I have no idea what this means given just this message.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp:73
+  // 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();

This is also very confusing.


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

[Tibor Brunner via Phabricator via cfe-commits]

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

[Tibor Brunner via Phabricator via cfe-commits]

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

[Tibor Brunner via Phabricator via cfe-commits]

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] D33825: [clang-tidy] signal handler must be plain old function check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Aside from a few small nits, this looks reasonable. Have you run it over any 
large code bases that use signals to test the quality of the check?




Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:23
+extern "C" void cpp_signal_handler(int sig) {
+  // warning: do not use C++ representations in signal handlers
+  throw "error message";

The warning text should match the actual warning.



Comment at: 
test/clang-tidy/cert-signal-handler-must-be-plain-old-function.cpp:56-57
+  // 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();

Can you move these to be closer to where the actual note appears? It makes it 
easier to ensure that the diagnostic appears on the proper line.


Repository:
  rL LLVM

https://reviews.llvm.org/D33825



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

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Eniko Donatella Toth via Phabricator via cfe-commits]

NorenaLeonetti updated this revision to Diff 113652.

Repository:
  rL LLVM

https://reviews.llvm.org/D33825

Files:
  clang-tidy/cert/CERTTidyModule.cpp
  clang-tidy/cert/CMakeLists.txt
  clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
  clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cert-msc54-cpp.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cert-signal-handler-must-be-plain-old-function.cpp

Index: test/clang-tidy/cert-signal-handler-must-be-plain-old-function.cpp
===
--- /dev/null
+++ test/clang-tidy/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: 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);
+  register short s;
+  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: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -37,6 +37,7 @@
cert-flp30-c
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
+   cert-msc54-cpp
cert-oop11-cpp (redirects to misc-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature
cppcoreguidelines-interfaces-global-init
Index: docs/clang-tidy/checks/cert-msc54-cpp.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cert-msc54-cpp.rst
@@ -0,0 +1,34 @@
+.. title:: clang-tidy - cert-msc54-cpp
+
+cert-msc54-cpp
+==
+
+This check will give a warning if a signal handler is not defined
+as an `extern "C"` function or if the 

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Johann Klähn via Phabricator via cfe-commits]

jklaehn added inline comments.



Comment at: docs/ReleaseNotes.rst:62
 
+<<< 2f301f50187ede4b9b8c7456ac4a67b9f7418004
 - Renamed checks to use correct term "implicit conversion" instead of "implicit

You missed a conflict marker here (and below in line 149).


Repository:
  rL LLVM

https://reviews.llvm.org/D33825



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

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Eniko Donatella Toth via Phabricator via cfe-commits]

NorenaLeonetti updated this revision to Diff 113651.
Herald added a subscriber: JDevlieghere.

Repository:
  rL LLVM

https://reviews.llvm.org/D33825

Files:
  clang-tidy/cert/CERTTidyModule.cpp
  clang-tidy/cert/CMakeLists.txt
  clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
  clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cert-msc54-cpp.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cert-signal-handler-must-be-plain-old-function.cpp

Index: test/clang-tidy/cert-signal-handler-must-be-plain-old-function.cpp
===
--- /dev/null
+++ test/clang-tidy/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: 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);
+  register short s;
+  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: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -37,6 +37,7 @@
cert-flp30-c
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
+   cert-msc54-cpp
cert-oop11-cpp (redirects to misc-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature
cppcoreguidelines-interfaces-global-init
Index: docs/clang-tidy/checks/cert-msc54-cpp.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cert-msc54-cpp.rst
@@ -0,0 +1,34 @@
+.. title:: clang-tidy - cert-msc54-cpp
+
+cert-msc54-cpp
+==
+
+This check will give a warning if a signal handler is not defined
+as 

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Eniko Donatella Toth via Phabricator via cfe-commits]

NorenaLeonetti marked 16 inline comments as done.
NorenaLeonetti added a comment.

In https://reviews.llvm.org/D33825#772688, @aaron.ballman wrote:

> This check is an interesting one. The rules around what is signal safe are 
> changing for C++17 to be a bit more lenient than what the rules are for 
> C++14. CERT's rule is written against C++14, and so the current behavior 
> matches the rule wording. However, the *intent* of the rule is to ensure that 
> only signal-safe functionality is used from a signal handler, and so from 
> that perspective, I can imagine a user compiling for C++17 to want the 
> relaxed rules to still comply with CERT's wording. What do you think?


I think your concerns are right. I've checked the upcoming changes in the C++17 
standard draft and I'll add some logic to the code to have those relaxed rules 
at least partly fulfilled.
For now, I uploaded the changes required for this check.


Repository:
  rL LLVM

https://reviews.llvm.org/D33825



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

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This check is an interesting one. The rules around what is signal safe are 
changing for C++17 to be a bit more lenient than what the rules are for C++14. 
CERT's rule is written against C++14, and so the current behavior matches the 
rule wording. However, the *intent* of the rule is to ensure that only 
signal-safe functionality is used from a signal handler, and so from that 
perspective, I can imagine a user compiling for C++17 to want the relaxed rules 
to still comply with CERT's wording. What do you think?




Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:22
+namespace {
+internal::Matcher notExternCSignalHandler() {
+  return functionDecl(unless(isExternC())).bind("signal_handler");

Since this is only needed once and is quite succinct, I'd just lower it into 
its usage.



Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:47
+
+internal::Matcher signalHandlerWithCallExpr() {
+  return functionDecl(hasDescendant(callExpr()))

Same here.



Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:52
+
+internal::Matcher allCallExpr() {
+  return decl(forEachDescendant(callExpr().bind("call")));

And here.



Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:83
+  const auto *Call = Match.getNodeAs("call");
+  const auto *Func = Call->getDirectCallee();
+  if (!Func || !Func->getDefinition())

Don't use `auto` for this one, since the type is neither complex nor spelled 
out in the initialization.



Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:84
+  const auto *Func = Call->getDirectCallee();
+  if (!Func || !Func->getDefinition())
+continue;

Can use `isDefined()` instead of `getDefinition()`. Then you can store off the 
`FunctionDecl *` for the definition and use it below in the call to 
`hasCxxRepr()`.



Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:102-103
+  }
+  SourceLocation callLoc() { return FunctionCall; }
+  SourceLocation cxxRepLoc() { return CxxRepresentation; }
+

These functions can be marked `const`.



Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:141
+diag(SignalHandler->getLocation(),
+ "use 'external C' prefix for signal handlers");
+diag(Result.Nodes.getNodeAs("signal_argument")->getLocation(),

'extern \"C\"'' instead of 'external C'. I'd probably reword it to: `"signal 
handlers must be 'extern \"C\""`



Comment at: clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp:149
+diag(SingalHandler->getLocation(),
+ "do not use C++ representations in signal handlers");
+if (const auto *CxxStmt = Result.Nodes.getNodeAs("cxx_stmt"))

representations -> constructs

(same below)



Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:24
+extern "C" void cpp_signal_handler(int sig) {
+//warning: do not use C++ representations in signal handlers
+  throw "error message";

Space between // and warning; indent the comment.



Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:29
+void install_cpp_signal_handler() {
+if (SIG_ERR == std::signal(SIGTERM, cpp_signal_handler))
+  return;

Indent the code.


Repository:
  rL LLVM

https://reviews.llvm.org/D33825



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

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/cert/CERTTidyModule.cpp:64
+"cert-msc54-cpp");
 // C checkers
 // DCL

checker -> check.



Comment at: docs/ReleaseNotes.rst:73
+
+  Checks if a signal handler is not a plain old function.
+

Probably C++ should be mentioned for clarity.



Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:7
+This check will give a warning if a signal handler is not defined
+as an 'extern C' function or if the declaration of the function
+contains C++ representation.

'extern C' -> ``extern "C"``.



Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:21
+}
+
+

Unnecessary empty line.



Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:32
+}
+
+

Unnecessary empty line.



Comment at: docs/clang-tidy/checks/cert-msc54-cpp.rst:34
+
+This check corresponds to the CERT CPP Coding Standard rule
+`MSC54-CPP. A signal handler must be a plain old function

Please replace CPP with C++ as in similar checks documentation.


Repository:
  rL LLVM

https://reviews.llvm.org/D33825



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

[PATCH] D33825: [clang-tidy] signal handler must be plain old function check

[Eniko Donatella Toth via Phabricator via cfe-commits]

NorenaLeonetti created this revision.
NorenaLeonetti added a project: clang-tools-extra.
Herald added subscribers: xazax.hun, mgorny.

Based on CERT-MSC54-CPP


Repository:
  rL LLVM

https://reviews.llvm.org/D33825

Files:
  clang-tidy/cert/CERTTidyModule.cpp
  clang-tidy/cert/CMakeLists.txt
  clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
  clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cert-msc54-cpp.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cert-signal-handler-must-be-plain-old-function.cpp

Index: test/clang-tidy/cert-signal-handler-must-be-plain-old-function.cpp
===
--- /dev/null
+++ test/clang-tidy/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: use 'external C' prefix for signal handlers [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++ representations in signal handlers [cert-msc54-cpp]
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: note: C++ representation 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++ representations in signal handlers [cert-msc54-cpp]
+  // CHECK-MESSAGES: :[[@LINE-11]]:3: note: function called here
+  // CHECK-MESSAGES: :[[@LINE-23]]:3: note: C++ representation 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++ representations 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);
+  register short s;
+  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: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -22,6 +22,7 @@
cert-flp30-c
cert-msc30-c (redirects to cert-msc50-cpp) 
cert-msc50-cpp
+   cert-msc54-cpp
cert-oop11-cpp (redirects to misc-move-constructor-init) 
cppcoreguidelines-interfaces-global-init
cppcoreguidelines-no-malloc
Index: docs/clang-tidy/checks/cert-msc54-cpp.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cert-msc54-cpp.rst
@@ -0,0 +1,36 @@
+.. title:: clang-tidy - cert-msc54-cpp
+
+cert-msc54-cpp