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:
> > > 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++.
> Okay, I could see that. Would you like to collaborate with the author of
> D33825 to see if you can produce a combined check? Or would you prefer to
> wait for that review to land for C++ and then modify it for C? (Or some other
> approach entirely?)
For me it looks better to pull in code from the other review. I found multiple
issues with it but the detection code is usable here. It should be better
however to first commit a simple version of the checker, for C functions only,
and extend it in smaller patches.
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
