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:
> 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.
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
