This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Support C++14 in bugprone-signal-handler.
ClosedPublic

Authored by balazske on Feb 4 2022, 7:09 AM.

Details

Summary

Check bugprone-signal-handler is improved to check for
C++-specific constructs in signal handlers. This check is
valid until C++17.

Diff Detail

Event Timeline

balazske created this revision.Feb 4 2022, 7:09 AM
balazske requested review of this revision.Feb 4 2022, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2022, 7:09 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
13–14

return LangOpts.CPlusPlus17 == 0;

13–14

Is there any utility/performance to be gained by combining these into a single matcher?
e.g.

callExpr(callee(SignalFunction), anyOf(hasArgument(1, HandlerExpr), hasArgument(1, HandlerLambda)))

(not sure if that is syntactically valid, but you get my point)

13–14

drop braces on 'then' block per style guide

13–14

drop braces per style guide

14

isInGlobalNamespace might be a better name?

14

How can the first expression here ever be false when
we rejected C++17 in the isLanguageVersionSupported
override?

23–31

Style guide says no braces on single statement blocks

36

Hrm, I was curious what getStmtClassName() does, but there doesn't seem to be any doxygen on it or the underlying data structure it accesses. Drilling into it with the IDE seems to say that it's a bunch of enums and corresponding names for each statement kind?

balazske updated this revision to Diff 406476.Feb 7 2022, 8:22 AM

Rebase, small fixes.

balazske marked 2 inline comments as done.Feb 7 2022, 8:22 AM
balazske added inline comments.
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
13–14

I think that readability does not get better if the braces are removed here, specially if only from one branch. The rules here are not strict in this case.

14

I was thinking for the case when this check supports C++17 too. The change can be added later, this makes current code more readable.

23–31

Improved the code but I think if an if branch has braces then the else should have too.

36

It should return the statement class name, comes somehow from TableGen. Here if the class name contains CXX it is recognized as a C++ statement. This check is not fully accurate (for example LambdaExpr is missing) but there is likely some other CXX statement around.

whisperity added a subscriber: whisperity.
whisperity added inline comments.
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
13–14

Or perhaps

callExpr(callee(SignalFunction), hasArgument(1, expr(anyOf(HandlerExpr, HandlerLambda))));
13–14

problem?

13–14

🔮 Ever since D98635, diag() of Clang-Tidy should support full undersquigglying of entities in diagnostics. Perhaps if you're working on this check already, it would be a nice addition to this check's output! All you need to do is << /* something that is a SourceRange */; into the result of a diag() call. 😃

13–14

(🔮 Ditto.)

14

Is this feature not available in (some parent class of) FunctionDecl?

14

Okay, this is interesting... Why is Ctx not const? Unless getSomething() is changing things (which is non-intuitive then...), as far as I can tell, you're only reading from Ctx.

14

(Nit: maybe it is better without this newline, to visibly demarcate the "local state init "block"" of the function.)

14

Either this... 🗡️

14

🗡️ ... or this.

14

(🗡️ Ditto.)

14

(True. I think @LegalizeAdulthood's comment can be marked as Done.)

14–15
16

Are these objects cleaned up properly in their destructor (same question for DynTypeNodeList)?

16

What is happening here? Why is the last parameter a callback(?) taking a bool? Why is the lambda empty? I see it is a std::function, can't we instead pass an empty function object there, and guard against calling it in checkFunction, making things a bit more explicit?

17

SourceRange itself should have a sentinel query. Why is only the beginning of it interesting?

17

I am trying to find some source for this claim but so far hit a blank. 😦 Could you please elaborate where this information comes from? Most notably, std::signal on CppReference makes no mention of this, which would be the first place I would expect most people to look at.

Maybe this claim is derived from the rule that signal handlers MUST have C linkage? (If so, is there a warning against people setting C++-linkage functions as signal handlers in this check in general?)

18–20

(🔮 Ditto.)

19

(Perhaps a >= C++20 fixme that this query here might not handle modules well?)

19–21

size != 1 could still mean size == 0 in which case taking an element seems dangerous. Is triggering such UB possible here?

21

(🗡️ Ditto.)

41–43
44–45

I think this is a fine moment to use the Remark diagnostic category! ✨ It is very likely that the average client will not look into Clang internals here... (It is dubious whether or not emitting this diagnostic is meaningful in the first place, but at least it helps with some debugging/understanding for those who wish to dig deep.)

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
27

Reading "set type" below where this is used, I would've thought this will be a specific instantiation of SparseSet at first.

39–40

What is a "function property"? (Or is that an implementation detail of the check?)

46–57
48

(Nit: to keep consistency with others and to ensure documentation renders properly, use @p checkFunction instead of backtick.)

50

(Format ditto.)

clang-tools-extra/docs/ReleaseNotes.rst
115–116

swap supports and partially

clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst
8 ↗(On Diff #406476)

(Is it the same for C all across the board?)

10–19 ↗(On Diff #406476)

I would turn these into bullet points:

  • For C, it is checked [...]
  • Up to and including C++14, [...]
  • Since C++17, [...]
    • C++17 and newer standards are not implemented in this check.
12–16 ↗(On Diff #406476)

Turning these into bullet points will make it easier for the clients to read and you can optimise away this troublesome sentence.

  • Up to and including C++14, there are several language constructs which are unsafe to call from a signal handler.
    • The signal handler, and every function it calls, MUST have C linkage.
    • Called functions must be asynchronous-safe.
    • etc.
16 ↗(On Diff #406476)
22 ↗(On Diff #406476)
24 ↗(On Diff #406476)
25–28 ↗(On Diff #406476)

I would rephrase this paragraph. You say that only the names are considered but the "against what" is not clear at this moment. Start with the expected outcome, the "result" function of this consideration:

Asnychronous-safety is determined by comparing the function's name against a set of known functions. In addition, the function must be a system call. The (possible) arguments passed to the function are not checked.

Just as in D118370, I'm not exactly sure what is a system call here, as printf and memcpy are (standard) library functions. Either this system call terminology should be dropped in favour of standard function perhaps, or the details of this be made explicit. Looking at the implementation of the check, the predicate for system call is: comes from a system header include + is in a global namespace + ???.

36 ↗(On Diff #406476)
40–53 ↗(On Diff #406476)

Again, this could use a re-working as a list of bullet-points.

42 ↗(On Diff #406476)

(I'd repeat the external URL and make CERT SIG30-C rule clickable.)

whisperity added inline comments.Mar 1 2022, 3:53 AM
clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst
44–47 ↗(On Diff #406476)

Nit: Perhaps the link could be worked into the POSIX.1-2017 somehow?

that is listed in POSIX.1-2017 (§ 2.4.3. Signal actions).

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
21 ↗(On Diff #406476)

I really like that this trivial bit stops being repeated! Is this what the SkipPathEnd does in practice?

35 ↗(On Diff #406476)

I went back to the implementation but I am still unsure what is being exercised by this change. Is it that we are not sensitive to conditionals and still think that the signal call will set handler_extern to be a handler no matter what, and thus we produce diagnostics? I think this is worthy of a comment in the file itself, because as far as I can tell there are no matchers that try to catch ifs in the check's implementation.

168–172 ↗(On Diff #406476)

What is the expected behavioural change by this? I don't see lines referring these signal() calls in other test cases. Should there be diagnostics but not yet implemented? Or should there just be no crashes? Both cases I believe should be mentioned in the comments.

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp
5 ↗(On Diff #406476)

Stale comment. Or perhaps this is important?

59 ↗(On Diff #406476)
balazske added inline comments.Mar 22 2022, 1:31 AM
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
17

This check is made to comply with a CERT rule MSC54-CPP. According to this only the subset of C and C++ should be used in a signal handler, and it should have extern "C" linkage. This does not allow lambda functions because the linkage restriction (if not others). (From C++17 on other rules apply, this check is not applicable for such code.)

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
48

I see that the backtick character is used at other places. It should work with Doxygen. The \c is the normal Doxygen command to make code-style text format, \p is used for function parameters.

Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 1:31 AM
whisperity added inline comments.Mar 29 2022, 2:24 AM
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
17

Ah, right, I found it a bit further down the page:

Signal handlers are expected to have C linkage and, in general, only use the features from the common subset of C and C++. It is implementation-defined if a function with C++ linkage can be used as a signal handler. (until C++17)

balazske updated this revision to Diff 418876.Mar 29 2022, 7:50 AM

rebase, applied the review comments

balazske marked 29 inline comments as done.Mar 29 2022, 8:01 AM

Code is updated, documentation (rst) not yet. I want to use \ format of tags in the code comments. Many review comments are now out of place (because the list of functions was moved around in the file.)

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
14

It does only reading, but getParentMapContext does not work with const.

16

I think it should work if there is not a function for deallocating.

16

Documentation of the function checkFunction tells what is the last parameter for: It is used to display the call chain notes (works as callback). A function object is used because then more information can be passed into it (instead of additional parameters to checkFunction) and it is possible to pass an empty function if needed. Here this function is not used, it is empty.

19–21

size is exactly 1 after the if

44–45

The statement class is often relatively easy to understand without looking into source code. Still it is a hint only, but there is no easy way to get a description for any statement class (without hard-coding into the checker).

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
39–40

I meant a property of the function in everyday sense, for example is it "extern C" or not, const or not, and similar (anything that is not in the body part).

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
21 ↗(On Diff #406476)

Yes SkipPathEnd is added to remove these redundant notes.

balazske marked 5 inline comments as done.Mar 29 2022, 8:04 AM
balazske added inline comments.
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
255

This will be removed (FunctionDecl::isGlobal should work for this purpose).

balazske updated this revision to Diff 421103.Apr 6 2022, 11:56 PM

Removed a left comment and improved another comment.

balazske marked an inline comment as done.Apr 6 2022, 11:57 PM
balazske updated this revision to Diff 422419.Apr 13 2022, 12:34 AM

Rebase to newer 'main' branch.

After adding improvements to the documentation, I think this will be good to go, and thank you! Perhaps just for a safety measure you could run it on a few projects (LLVM itself?) to ensure we didn't miss a case where it might magically crash, but I wonder how many specifically "C++14" projects will use signal handlers in the first place.

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
352

Aren't these bools?

njames93 added inline comments.Apr 26 2022, 1:51 AM
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
13–14

Is this check valid on Objective-C code?

477

s/with other than/without

496

This just feels very hacky and it's non exhaustive, would lambda for a start as that isn't prefixed internally with CXX.

After adding improvements to the documentation, I think this will be good to go, and thank you! Perhaps just for a safety measure you could run it on a few projects (LLVM itself?) to ensure we didn't miss a case where it might magically crash, but I wonder how many specifically "C++14" projects will use signal handlers in the first place.

It did not crash on llvm, nor on our internal test set.

balazske added inline comments.Apr 28 2022, 4:01 AM
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
13–14

I do not know if the check is applicable to Objective-C, it may depend on if the C standard (or POSIX) is applicable to Objective-C.

352

This is a 1-bit bit field member.

496

This is somewhat "hacky" but is more future-proof if new Stmt classes are added. I extend the list with isa checks for classes in ExprCXX.h and StmtCXX.h. But it is likely that when a new C++-only node is added this list is not updated (maybe no problem because new things are for later than C++17).

balazske updated this revision to Diff 425753.Apr 28 2022, 5:44 AM

Applied the review comments.

@njames93 What do you think about the current approach? It will under-approximate the problem-inducing node set but at least cover what we know about C++ now.

(@balazske please mark "Done" the answered discussion threads.)

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
285–297

isa<> is a variadic template since a few major releases ago, i.e. isa<T1, T2, T3>(X) is supported too.

(Also, please order this alphabetically, so it's easier to read and insert into at later edits.)

286

Is CUDA built upon C++-specific features?

balazske updated this revision to Diff 429205.May 13 2022, 5:09 AM
balazske marked 8 inline comments as done.

Using one isa statement for all class names.
Changed the documentation.

balazske added inline comments.May 15 2022, 11:58 PM
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
286

It looks like no, but the class CUDAKernelCallExpr resides in ExprCXX.h so I think it is a C++ specific class. This may be a C++ extension.

balazske marked an inline comment as done.Jun 13 2022, 12:32 AM

Ping
I have a question with Objective-C support.
@njames93 Could you look at this again?

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
13–14

I could not find out if it is valid for Objective-C, probably not? But there are other checkers (for example in the concurrency module) that look POSIX or runtime-library specific and these do not contain check for ObjC language.

LegalizeAdulthood requested changes to this revision.Jun 22 2022, 2:09 PM

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

  • fold your changes into the appropriate subdirs, stripping the module prefix from new files.
  • make the target check-clang-extra to validate your tests
  • make the target docs-clang-tools-html to validate any docs changes
This revision now requires changes to proceed.Jun 22 2022, 2:09 PM
balazske updated this revision to Diff 439751.Jun 24 2022, 7:32 AM

updated for new directory layout, fixed documentation

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
282

I'm not a fan of this macro, and we're only using it once as the argument to isa<>; is there some problem with writing it inline?

clang-tools-extra/docs/ReleaseNotes.rst
114

Please keep the section sorted by check, so this should be inserted before bugprone-use-after-move

clang-tools-extra/test/clang-tidy/checkers/bugprone/signal-handler.cpp
2

Prefer -isystem %clang_tidy_headers instead of -isystem %S/../Inputs/Headers

balazske marked 3 inline comments as done.Jun 27 2022, 1:03 AM
balazske added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
114

The list is already not sorted.

balazske updated this revision to Diff 440118.Jun 27 2022, 1:05 AM
balazske marked an inline comment as done.

Applied the review comments.

clang-tools-extra/docs/ReleaseNotes.rst
114

sort by check name

balazske updated this revision to Diff 442024.Jul 4 2022, 1:15 AM

Sorted the whole changed-check list in release notes.

balazske marked an inline comment as done.Jul 4 2022, 1:16 AM
balazske marked 3 inline comments as done.Aug 1 2022, 3:47 AM
whisperity accepted this revision.Aug 1 2022, 3:54 AM

Alright, I think we should have this in and let C++17 things to be future work. Please see the inline comment, but otherwise this should be good enough. Can always improve in a future version. 😉

LLVM 15 has branched off, so this should be rebased for the last time against the current main branch, just to ensure everything is good.

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
284–298

I have an overarching solution idea for this, @balazske, which requires a separate patch independently of this. (I believe this will not be a hard thing to implement, just tedious, because of all the classes that need to be changed.)

Let's consider adding a new member function to Decl, Stmt, Type, etc. instances. While bool isCXX() const seems tempting, I believe it should be something like bool isAvailableIn(const LangOptions&) const, which returns whether a particular node (representing a language feature) is available in a particular language option set. And this way, we could query if a node is available in C++ (whichever version) and not in C, in which case the check can deduce that it is "C++-only".

This way, the future maintenance burden will be completely lifted from the check, and the AST library/API gets a useful generic feature that we can start using elsewhere, too!


So, e.g., LambdaExpr will have:

bool LambdaExpr::isAvailableIn(const LangOptions& LO) const
{
  return LO.CPlusPlus11;
}
whisperity accepted this revision.Aug 9 2022, 6:00 AM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 10 2022, 3:01 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

@balazske This is failing on https://lab.llvm.org/buildbot/#/builders/139/builds/26335 - do you have a fix in progress or should I revert?

@balazske This is failing on https://lab.llvm.org/buildbot/#/builders/139/builds/26335 - do you have a fix in progress or should I revert?

Fix is not in progress, it may take some days.

@balazske This is failing on https://lab.llvm.org/buildbot/#/builders/139/builds/26335 - do you have a fix in progress or should I revert?

Fix is not in progress, it may take some days.

I believe the problem is due to target specific default settings. Therefore, adding an explicit target, e.g. -target x86_64-unknown-unknown, to the %check_clang_tidy call should fix the problem.

The target option was added, but I can not verify if the change fixes all the buildbots, we will see if it works.