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<CXXUnresolvedConstructExpr>(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
