This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] signal handler must be plain old function check
Needs RevisionPublic

Authored by bruntib on Jun 2 2017, 5:29 AM.

Diff Detail

Event Timeline

NorenaLeonetti created this revision.Jun 2 2017, 5:29 AM
Eugene.Zelenko added inline comments.Jun 2 2017, 10:25 AM
clang-tidy/cert/CERTTidyModule.cpp
64 ↗(On Diff #101191)

checker -> check.

docs/ReleaseNotes.rst
73 ↗(On Diff #101191)

Probably C++ should be mentioned for clarity.

docs/clang-tidy/checks/cert-msc54-cpp.rst
7 ↗(On Diff #101191)

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

21 ↗(On Diff #101191)

Unnecessary empty line.

32 ↗(On Diff #101191)

Unnecessary empty line.

34 ↗(On Diff #101191)

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

aaron.ballman edited edge metadata.Jun 5 2017, 6:11 AM

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?

clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
22 ↗(On Diff #101191)

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

47 ↗(On Diff #101191)

Same here.

52 ↗(On Diff #101191)

And here.

83 ↗(On Diff #101191)

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

84 ↗(On Diff #101191)

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().

102–103 ↗(On Diff #101191)

These functions can be marked const.

141 ↗(On Diff #101191)

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

149 ↗(On Diff #101191)

representations -> constructs

(same below)

docs/clang-tidy/checks/cert-msc54-cpp.rst
24 ↗(On Diff #101191)

Space between // and warning; indent the comment.

29 ↗(On Diff #101191)

Indent the code.

whisperity edited the summary of this revision. (Show Details)Jul 6 2017, 5:21 AM
whisperity added subscribers: gsd, o.gyorgy, dkrupp, whisperity.
alexfh requested changes to this revision.Jul 11 2017, 6:42 AM
This revision now requires changes to proceed.Jul 11 2017, 6:42 AM
NorenaLeonetti marked 16 inline comments as done.Sep 2 2017, 6:19 AM

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.

NorenaLeonetti edited edge metadata.
jklaehn added a subscriber: jklaehn.Sep 2 2017, 9:48 AM
jklaehn added inline comments.
docs/ReleaseNotes.rst
62 ↗(On Diff #113651)

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

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?

docs/clang-tidy/checks/cert-msc54-cpp.rst
23 ↗(On Diff #113652)

The warning text should match the actual warning.

test/clang-tidy/cert-signal-handler-must-be-plain-old-function.cpp
56–57 ↗(On Diff #113652)

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.

alexfh removed a reviewer: alexfh.Sep 10 2017, 6:00 AM
alexfh added a subscriber: alexfh.
bruntib commandeered this revision.Aug 28 2020, 7:37 AM
bruntib added a reviewer: NorenaLeonetti.
bruntib added a subscriber: bruntib.

I'll rebase this patch and continue its development.

bruntib updated this revision to Diff 288606.Aug 28 2020, 7:49 AM

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.

bruntib updated this revision to Diff 288617.Aug 28 2020, 8:30 AM

Update lincense.

jfb requested changes to this revision.Aug 28 2020, 8:46 AM

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

clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
23

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

58

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

74

This is also very confusing.

This revision now requires changes to proceed.Aug 28 2020, 8:46 AM
mgehre removed a subscriber: mgehre.Aug 28 2020, 12:44 PM
bruntib updated this revision to Diff 288774.Aug 29 2020, 6:34 AM

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";
^
bruntib added inline comments.Aug 29 2020, 6:40 AM
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
23

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?

bruntib updated this revision to Diff 288777.Aug 29 2020, 8:13 AM

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"

jfb requested changes to this revision.Aug 29 2020, 3:47 PM

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]).
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
84

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

clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst
15

'extern', not 'external'

23

Update the example too.

clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
74

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

91

_Thread_local isn't signal safe per p0270.

114

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

This revision now requires changes to proceed.Aug 29 2020, 3:47 PM
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.

clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
42

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.

44

Diagnostics in clang-tidy should start with a lowercase letter

91

Similar concerns here.