aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:131
+ }
+ /*FunctionCallCollector Collector{[&CalledFunctions](const CallExpr *CE) {
+ if (isa<FunctionDecl>(CE->getCalleeDecl()))
----------------
Remove commented-out code?
================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:162
+
+ if (StrictConformingFunctions.count(II->getName()))
+ return true;
----------------
You may want a FIXME here that this will eventually be wrong in C++ mode.
Consider declarations like:
```
namespace foo {
void abort();
inline namespace bar {
void quick_exit(int);
}
}
namespace {
void signal();
}
```
If we form a FunctionDecl to any of those functions, we shouldn't count those
as the conforming functions.
================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:175
+ "calling it from a signal handler may be dangerous")
+ << CalledFunction->getNameAsString();
+ diag(SignalCall->getSourceRange().getBegin(),
----------------
You can pass `CalledFunction` directly -- any `NamedDecl` gets automatically
formatted by the diagnostics engine. You should also drop the single quotes
around `%0` as they'll be automatically added by the diagnostics engine.
================
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:
> > balazske wrote:
> > > aaron.ballman wrote:
> > > > 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.
> > > The function declaration is not often a macro name so there is no
> > > "expansion location" or the same as the original location. My concern was
> > > that if there is a declaration of system call function in a source file
> > > (like `void abort(void);` in .c file) for any reason, we may find this
> > > declaration instead of the one in the header file, if not looping over
> > > the declaration chain.
> > > The function declaration is not often a macro name so there is no
> > > "expansion location" or the same as the original location.
> >
> > Agreed.
> >
> > > My concern was that if there is a declaration of system call function in
> > > a source file (like `void abort(void);` in .c file) for any reason, we
> > > may find this declaration instead of the one in the header file, if not
> > > looping over the declaration chain.
> >
> > Typically, when a C user does something like that, it's because they're
> > explicitly *not* including the header file at all (they're just forward
> > declaring the function so they can use it) and so we'd get the logic wrong
> > for them anyway because we'd never find the declaration in the system
> > header.
> >
> > Using the canonical declaration is more typical and would realistically
> > fail only in circumstances like forward declaring a system header function
> > and then later including the system header itself. That said, I suppose
> > your approach is defensible enough. Redeclaration chains are hopefully
> > short enough that it isn't a performance hit.
> I changed back to the original code to search the entire redeclaration chain.
> Otherwise it can be fooled with declarations before or after inclusion of the
> system header. Such declarations were added to the test file (it did not pass
> if `isExpansionInSystemHeader` was used).
I don't think that's necessary (we aren't consistent about whether we check the
entire redecl chain, so I worry about inconsistent behavior between checks). If
you are looking at the canonical declaration (instead of just any declaration),
you'll always be looking at the *first* declaration encountered, which is
generally sufficient.
```
// a.c
#include <sysheader.h> // Most common case will be inclusion directly from a
header, this works fine
// b.c
extern void sysfunc(void); // Next most common case will be extern declaration,
can't catch this with either approach.
// c.c
#include <sysheader.h> // Canonical declaration, so this works
extern void sysfunc(void); // redecl won't matter
// d.c
extern void sysfunc(void); // Canonical declaration, so this fails
#include <sysheader.h> // But at the same time, who does this?
```
I don't insist on a change, but as a mental note to myself and other authors,
we should probably try to have a more consistent policy here.
================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:112
+
+ Finds functions registered as signal handlers that call non asynchronous-safe
+ functions.
----------------
Can you mention that this check is currently for C only?
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:1-17
+.. title:: clang-tidy - cert-sig30-c
+
+cert-sig30-c
+============
+
+Finds functions registered as signal handlers that call non asynchronous-safe
+functions. User functions called from the handlers are checked too, as far as
----------------
baloghadamsoftware wrote:
> Please add at least one minimal code example. (E.g. from the tests.)
Also, please document that the check is currently limited to C code.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/cert-sig30-c_cpp.h:1
+//===--- Header for test cert-sig30-c.cpp -----------------------*- C++
-*-===//
+//
----------------
I think this file can be removed entirely for now.
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
