aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:33 + + for (const FunctionDecl *D : Node.redecls()) + if (D->getASTContext().getSourceManager().isInSystemHeader( ---------------- balazske wrote: > aaron.ballman wrote: > > I'm not certain I understand why we're looking through the entire > > redeclaration chain to see if the function is ever mentioned in a system > > header. I was expecting we'd look at the expansion location of the > > declaration and see if that's in a system header, which is already handled > > by the `isExpansionInSystemHeader()` matcher. Similar below. > This function is called from ` SignalHandlerCheck::check` when any function > call is found. So the check for system header is needed. It was unclear to me > what the "expansion location" means but it seems to work if using that > expansion location and checking for system header, instead of this loop. I > will update the code. > This function is called from SignalHandlerCheck::check when any function > call is found. So the check for system header is needed. The check for the system header isn't what I was concerned by, it was the fact that we're walking the entire redeclaration chain to see if *any* declaration is in a system header that I don't understand the purpose of. > It was unclear to me what the "expansion location" means but it seems to work > if using that expansion location and checking for system header, instead of > this loop. I will update the code. The spelling location is where the user wrote the code and the expansion location is where the macro name is written, but thinking on it harder, that shouldn't matter for this situation as either location would be in the system header anyway. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:13 +(for ``signal`` there are additional conditions that are not checked). +Every other system call is considered as non asynchronous-safe by the checker. + ---------------- balazske wrote: > aaron.ballman wrote: > > I would document this as: `Any function that cannot be determined to be an > > asynchronous-safe function call is assumed to be non-asynchronous-safe by > > the checker, including function calls for which only the declaration of the > > called function is visible.` > "including function calls for which only the declaration of the called > function is visible": Is this the better approach? The checker does not make > warning for such functions in the current state. Ooh, thank you for calling this out, you're right that I wasn't describing the current behavior. My thinking is: most system functions aren't safe to call within a signal handler and user-defined functions will eventually call a system function more often than they won't, so assuming a function for which you can't see the definition is not signal safe is a somewhat reasonable assumption, but may have false positives. However, under the assumption that most signal handlers are working as intended, then perhaps it's better to assume that the author of such unseen function bodies did the right thing as you're doing, but then you may have false negatives. Given that the CERT rules are about security, I think it's better to err on the side of more false positives than more false negatives, but it's open for debate. 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
