[PATCH] D83717: [clang-tidy] Add check fo SEI CERT item ENV32-C

[Endre Fülöp via Phabricator via cfe-commits]

gamesh411 marked 13 inline comments as done.
gamesh411 added a comment.

Note that there are no negative test cases that assert that we do NOT report in case a custom or an anonymous namespace is used. For that I would need a small patch in the testing infrastructure.
Patch needed in check_clang_tidy.py:

  --- a/clang-tools-extra/test/clang-tidy/check_clang_tidy.py
  +++ b/clang-tools-extra/test/clang-tidy/check_clang_tidy.py
  @@ -167,7 +167,7 @@ def run_test_once(args, extra_args):
         subprocess.check_output(
             ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
              '-check-prefixes=' + ','.join(check_fixes_prefixes),
  -           '-strict-whitespace'],
  +           '-strict-whitespace', '--allow-empty'],
             stderr=subprocess.STDOUT)
       except subprocess.CalledProcessError as e:
         print('FileCheck failed:\n' + e.output.decode())
  @@ -180,7 +180,7 @@ def run_test_once(args, extra_args):
         subprocess.check_output(
             ['FileCheck', '-input-file=' + messages_file, input_file_name,
              '-check-prefixes=' + ','.join(check_messages_prefixes),
  -           '-implicit-check-not={{warning|error}}:'],
  +           '-implicit-check-not={{warning|error}}:', '--allow-empty'],
             stderr=subprocess.STDOUT)
       except subprocess.CalledProcessError as e:
         print('FileCheck failed:\n' + e.output.decode())
  @@ -195,7 +195,7 @@ def run_test_once(args, extra_args):
         subprocess.check_output(
             ['FileCheck', '-input-file=' + notes_file, input_file_name,
              '-check-prefixes=' + ','.join(check_notes_prefixes),
  -           '-implicit-check-not={{note|warning|error}}:'],
  +           '-implicit-check-not={{note|warning|error}}:', '--allow-empty'],
             stderr=subprocess.STDOUT)
       except subprocess.CalledProcessError as e:
         print('FileCheck failed:\n' + e.output.decode())

And then I can assert the non-reports by adding the following runlines:

  // Functions in anonymous or custom namespace should not be considered as exit
  // functions.
  //
  // RUN: %check_clang_tidy -assume-filename=%s.cpp %s -check-suffix=CUSTOM \
  // RUN:     cert-env32-c %t -- -- -DCPPMODE -DTEST_NS_NAME=custom
  // CHECK-NOTES-CUSTOM-NOT: NO DIAGS
  //
  // RUN: %check_clang_tidy -assume-filename=%s.cpp %s -check-suffix=ANONYMOUS \
  // RUN:     cert-env32-c %t -- -- -DCPPMODE -DTEST_NS_NAME=''
  // CHECK-NOTES-ANONYMOUS-NOT: NO DIAGS



================
Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:70
+             .bind("handler_expr"));
+  Finder->addMatcher(
+      callExpr(IsRegisterFunction, HasHandlerAsFirstArg).bind("register_call"),
----------------
aaron.ballman wrote:
> I am not at all certain whether this is plausible, but I *think* this check can be almost entirely implemented in the matchers rather than having to do manual work. I think you can bind the argument node from the call to `at_quick_exit()` and then use `equalsBoundNode()` to find the function calls within the bound `functionDecl()` node.
> 
> However, if that's not workable, I think you can get rid of the `CalledFunctionsCollector` entirely and just use matchers directly within the `check()` function because by that point, you'll know exactly which AST nodes you want to traverse through.
I have investigated, and to do that recursive matching up to indeterminate depth all the while keeping the already matched functions in a set is not something I would implement with a single matcher. I could use a standalone matcher, but as far as I can understand I would need to implement a callback function for handling the matched results out of line for that as well, so I think that the ASTVisitor based solution is at least as good as a standalone ASTMatcher would be. Therefore I'd rather keep this solution.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-env32-c.c:31
+// --------------
+// C99 requires jmp_buf to be an array type.
+typedef int jmp_buf[1];
----------------
whisperity wrote:
> Which is the standard version this test file is set to analyse with? I don't see any `-std=` flag in the `RUN:` line.
Right now there is a run-line for handling it as C, and 2 others for handling it as C++ with different namespaces. The test cases themselves are not dependent on the standard used (AFAIK).  The comment contains a standard reference for C, but that is just the first time it was standardized in C so the mention is there for traceability and not to restrict the standard used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717