balazske marked an inline comment as done. balazske 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( ---------------- 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). 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
