[PATCH] D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C

Mon Nov 2 06:20:54 PST 2020

aaron.ballman added a comment.

In D87449#2358579 <https://reviews.llvm.org/D87449#2358579>, @balazske wrote:

> 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?

I have no issues putting this into the `bugprone` module and then aliasing to it from the `cert` module.

================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:33
+
+  for (const FunctionDecl *D : Node.redecls())
+    if (D->getASTContext().getSourceManager().isInSystemHeader(
----------------
balazske wrote:
> 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>
> ```
> There is not a documented guarantee (?) that canonical declaration is always the first one, and the last "d.c" case may happen:

Hmm, I am pretty sure this is our documentation on the topic: https://github.com/llvm/llvm-project/blob/ad8e900cb3c60873e954221816aafc6767222de2/clang/include/clang/AST/Redeclarable.h#L30 While it says "often", I'm not aware of a circumstance where that doesn't hold true.

As for the case you pointed out, yes, that's possible. However, to my original point, that's more consistent with the way clang-tidy checks currently work than what you're doing, which is why I've been pushing back on it. It's not a particularly worrisome case, but having inconsistent behavior between checks makes all checks harder for users to reason about because they have to remember which decision was made for any given check.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87449/new/

https://reviews.llvm.org/D87449