balazske added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118 + FunctionCallCollector Collector{[&CalledFunctions](const CallExpr *CE) { + if (isa<FunctionDecl>(CE->getCalleeDecl())) + CalledFunctions.push_back(CE); + }}; ---------------- aaron.ballman wrote: > balazske wrote: > > aaron.ballman wrote: > > > aaron.ballman wrote: > > > > balazske wrote: > > > > > aaron.ballman wrote: > > > > > > For correctness, I think you need to handle more than just calls to > > > > > > function declarations -- for instance, this should be just as > > > > > > problematic: > > > > > > ``` > > > > > > void some_signal_handler(int sig) { > > > > > > []{ puts("this should not be an escape hatch for the check); }(); > > > > > > } > > > > > > ``` > > > > > > even though the call expression in the signal handler doesn't > > > > > > resolve back to a function declaration. (Similar for blocks instead > > > > > > of lambdas.) WDYT? > > > > > I do not know how many other cases could be there. Probably this can > > > > > be left for future improvement, the checker is mainly usable for C > > > > > code then. There is a `clang::CallGraph` functionality that could be > > > > > used instead of `FunctionCallCollector` but the `CallExpr` for the > > > > > calls is not provided by it so it does not work for this case. Maybe > > > > > there is other similar functionality that is usable? > > > > Given that we want it in the CERT module, we should try to ensure it > > > > follows the rule as closely as we can. I went and checked what the C++ > > > > rules say about this and... it was interesting to notice that SIG30-C > > > > is not one of the C rules included by reference in C++ > > > > (https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336). > > > > > > > > It's not clear to me that this rule was accidentally tagged as > > > > `not-for-cpp` or not, so I'd say it's fine to ignore lambdas for the > > > > moment but we may have some follow-up work if CERT changes the rule to > > > > be included in C++. My recommendation is: make the check a C-only check > > > > for now, document it as such, and I'll ping the folks at CERT to see if > > > > this rule was mistagged or not. WDYT? > > > Ah, this rule really is a C-only rule, because > > > https://wiki.sei.cmu.edu/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function > > > is the C++ rule. So I think the SIG30-C checker should be run in C-only > > > mode and we can ignore the C++isms in it. > > > > > > FWIW, we have an ongoing discussion about MSC54-CPP in > > > https://reviews.llvm.org/D33825. > > Probably this checker can be merged with the other in D33825. According to > > cppreference (https://en.cppreference.com/w/cpp/utility/program/signal) the > > check for the called functions should be present for C++ too. And the other > > checker should do a similar lookup of called functions as this checker, > > including lambdas and C++ specific things. > While you would think that, it's a bit more complicated unfortunately. The > C++ committee has been moving forward with this paper http://wg21.link/p0270 > so that C++ is no longer tied to the same constraints as C. That may suggest > that separate checks are appropriate, or it may still mean we want to merge > the checks into one. I think it is more convenient to merge the two checkers. The visitation of called functions goes the same way, the support for C++ constructs should not cause problems if used with C code. The handling of a detected function can be different code for C and C++ mode but if there are similar parts code can be reused. Otherwise code of this checker would be a better starting point for "SignalHandlerMustBePlainOldFunctionCheck" because it handles detection of the `signal` function already better specially for C++. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87449/new/ https://reviews.llvm.org/D87449 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits