aaron.ballman 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);
+ }};
----------------
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?
================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp:52
+void test() {
+ std::signal(SIGINT, handler_abort);
+ std::signal(SIGINT, handler__Exit);
----------------
balazske wrote:
> aaron.ballman wrote:
> > I'd also like to see a test case where the handler to `signal` call is
> > itself not a function call:
> > ```
> > std::signal(SIGINT, [](int sig) {
> > puts("I did the bad thing this way"); // should be diagnosed, yes?
> > });
> > ```
> This is again a new case to handle. A new matcher must be added to detect
> this. But I am not sure how many other cases are there, or is it worth to
> handle all of them.
Let's punt on this until we hear back from CERT on whether this rule should be
supported in 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
