balazske added a comment. I think the name of this checker should be changed. It could in future not only check for the SIG30-C rule. (Plan is to include C++ checks too, and SIG31-C could be checked in this checker too.) It can be called "bugprone-signal-handler" instead?
================ 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: > > > > > > 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. > If I understand correctly, the problem is that other checkers already test > for "system calls" by checking only the canonical declaration (or > "isExpansionInSystemHeader"). It would be better to have a common function or > AST matcher for this purpose and update all checkers to use that (even in > clang static analyzer). My decision would be to use the current code, it is more safe. There is not a documented guarantee (?) that canonical declaration is always the first one, and the last "d.c" case may happen: ``` // d.h extern void sysfunc(void); // used by some construct in this file or just forgotten here // d.c #include "d.h" #include <sysheader.h> ``` 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
