On bitcoin v0.18.1, there is an assertion introduced by this change.
The TU that can be used for reproduction is src/script/interpreter.cpp.
Assertion message:

CTU loaded AST file: /home/gamesh411/bitcoin/src/script/script.cpp                                                                                                                                         
clang: /home/gamesh411/llvm-project/clang/lib/AST/ASTContext.cpp:4411: clang::QualType clang::ASTContext::getInjectedClassNameType(clang::CXXRecordDecl*, clang::QualType) const: Assertion `NeedsInjectedC
lassNameType(Decl)' failed.

In D83717#2370154, @NoQ wrote:

I don't think you actually need active support for invoking exit handlers path-sensitively at the end of main() in order to implement your checker. You can still find out in a path-insensitive manner whether any given function acts as an exit handler, like you already do. Then during path-sensitive analysis of that function you can warn at any invocation of exit().

I do believe that you want to implement this check as a path-sensitive check as long as you want any sort of interprocedural analysis (i.e., warn when an exit handler calls a function that calls a function ... that calls a function that calls exit() - and you do already have such tests). This is because of the following potential situation:

void foo(bool already_exiting) {
  if (!already_exiting)
    exit();
}

void bar() {
  foo(true);
}

void baz() {
  foo(false);
}

int main() {
  atexit(bar);
  return 0;
}

In this case bar() is an exit handler that calls foo() that calls exit(). However the code is correct and no warning should be emitted, because foo() would never call exit() when called from bar() specifically. Precise reasoning about such problems requires path-sensitive analysis. Of course it's up to you to decide what to do with these false positives - whether you'll be ok with having them, or choose to suppress with an imprecise heuristic - but that's one of the possible reasons to consider reimplementing the checker via path sensitive analysis that the static analyzer provides.

We've had a similar problem with the checker that warns on calling pure virtual functions in constructors/destructors (which is undefined behavior). Such checker had to be path sensitive in order to be interprocedural for the exact same unobvious reason. We've decided to re-implement it with path-sensitive analysis and now we're pretty happy about that decision.

Just to make sure we're on the same page -- the current approach is not flow-sensitive, and so my concern is that it won't report any true positives (not that it will be prone to false positives).

Sorry about that. You are absolutely right; what I was trying to say is CallGraph-based.

In D83717#2279263, @aaron.ballman wrote:

One of the concerns I have with this not being a cfg-only check is that most of the bad situations are not going to be caught by the clang-tidy version of the check.

...

Have you run this check over any large code bases to see if it currently catches any true positive diagnostics?

In D87830#2336572, @njames93 wrote:

Probably not quite as verbose but should do the job

// RUN: clang-tidy %s --checks=-*,my-check-to-test --warnings-as-errors=* -- -std=c++11

In D89528#2334795, @martong wrote:

What is the context here? Did it cause any crash/bug or were you just reading through the code for a good night sleep? :D

In D87830#2298198, @aaron.ballman wrote:

Do you have some thoughts about this, should this be pursued, or do you think the use-case is not relevant?

Update commit message

Tidy up commit message

Update commit msg with example

Reformat diagnostic message
Use explicit name longjmp instead of jump function
Fix liberal auto inside Collector

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())

Add abort and terminate handling
Extend tests to cover every exit functions
Extract matcher bind labels

only consider global and ::std scope handlers

CReduce did not manage to produce any meaningful result after a week worth of runtime (more than ~2000 lines of code still remaining after reduction). We could track this down by tracing the ExprEngine code that assigns the Undefined SVal but that seems a huge effort as well. That could be done by debugging the SVal-assigning statements, and setting conditional breakpoints (ie. only break when the value is Undefined). When a breakpoint is hit, we could dump the statement that triggered it and try to reason about the conditions at that point. I also recommend using the rr tool as it allows you to use fixed pointer values while debugging.

Thanks! LGTM now.

Aside from infrastructural questions which I am not qualified ( nor particularly knowledgeable :3 ) to address, this looks good to me.

rename file name in header

I would add one more test for the undefined case. Like a local array variable that is uninitialized. That could mirror some of the null-dereference cases.

rebase

I have updated the revision to use llvm::move algorithm (available thanks to @njames93).
Friendly ping :)

ensure lint in path

use llvm::move algorithm

arcanist misfire, see D83717

use llvm::move algorithm

I experienced 2 crashes with and without this patch using commit 1af9fc82132da7c876e8f70c4e986cc9c59010ee on master:
I have used the clang built on that revision to analyse itself, and also used the patched version (with this current revision applied) to do the same.

To further investigate the moving story, I also created a small example on Godbolt:
https://godbolt.org/z/5qMf1o

LGTM!

I am happy to see this change, as I could use it inside my clang-tidy check as well!

rename Offset -> Amount inside handleRandomIncrOrDecl
minor local variable renaming

apply review suggestions

• rename variables
• remove 1 assert
• rename tests

Thanks for reviewing this patch this quickly!
I have updated the diff according to your suggestions, but I will not land it till I run a llvm+clang analysis with it.
Do you think non-ctu mode is enough to test the stability?

It would be nice to see the measurements on llvm as this patch introduced some (IMO reasonable) asserts. Also in the unlikely case, there is an expression like 1 + iter, there could be more results.

rebase upon hotfix patch

extend test cases
add comments to non-obvious cases

move tests to one file

I am now experimenting with the suggestions. The other issues are on the worklist. Thanks for these so far.

In D83717#2155066, @njames93 wrote:

In D83717#2152762, @gamesh411 wrote:

In D83717#2150977, @njames93 wrote:

Alternatively you could do something like this, though it would be a pain https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment-gmock.cpp#L86

I have bitten the bullet, and have gone down this route. With relative numbering, the sections themselves are at least translation-invariant. Not the prettiest sight, tho.
Thanks!

I almost think it would be nice FileCheck supported some directive like:

// LINE-NAME: <LINENAME>

And corresponding check lines:

[[<LINENAME>]]
[[<LINENAME>+N]] 
[[<LINENAME>-N]]

Would result in the ability to write checks like this:

void longjmp_handler() {// LINE-NAME: LONGJMP
    longjmp(env, 255); 
}

...

void foo(){
  atexit(longjmp_handler);
   // CHECK-NOTES: :[[@LINE-1]]:3: warning: exit-handler potentially calls a jump function. Handlers should terminate by returning [cert-env32-c]
  // CHECK-NOTES: :[[LONGJMP]]:1: note: handler function declared here
  // CHECK-NOTES: :[[LONGJMP+1]]:3: note: jump function called here
}

Anyway that's a story for another day.

In D83717#2153246, @Eugene.Zelenko wrote:

In D83717#2153245, @gamesh411 wrote:

@Eugene.Zelenko I have just rebase-d, and seen that the release notes page itself was bumped to clang-tidy 12. I have added my check as a new check there. Should I also add the other subsections (like improvements in existing checks, and new check aliases), or authors will add them as needed?

No, that sections will be filled when related changes will be made. Release Notes are reset to blank state in master after branching release.

@Eugene.Zelenko I have just rebase-d, and seen that the release notes page itself was bumped to clang-tidy 12. I have added my check as a new check there. Should I also add the other subsections (like improvements in existing checks, and new check aliases), or authors will add them as needed?

rebase

tidy up release notes, make all new check entries uniform

add missing newline in release notes

.

fix documentation header

@steakhal @Eugene.Zelenko Thanks for checking this patch! I have tried my best to adhere to your suggestions.

In D83717#2150977, @njames93 wrote:

Alternatively you could do something like this, though it would be a pain https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment-gmock.cpp#L86

use move instead of copy
fix documentation issues
fix tests

Thanks @njames93 :) I have extended the check with notes, but I have to figure out how to appease file-check, so its still WIP until I do.

extend with notes
apply minor fixes
tests are WIP until I figure out how to properly use file-check

mention iterator header

use std::copy algorithm
polish

fix test diag location changes resulting from autoformatting

simplify VisitCallExpr

update includes

In addition, IMHO the test infra should then be extended to throw an exception, when a feature is referenced that is not mentioned anywhere. That would be helpful in the future.

Good thing you were looking inside the test infra code to catch this, kudos!

@Szelethus thanks for being watchful, appreciated c:

It will be interesting to see how different C and C++ projects will prove in terms of AST complexity, and Decl-size, but I understand that for now, these two options are necessary to not penalize C project analysis because of C++ AST complexity.

apply review suggestions

fix tidy diagnostic

The checker has been updated, the comments and the logic polished. I would still take a stab at this being a ClangSa checker (as opposed to being a Tidy check). What do you think?

fix detection logic
fix license header
add missing warning to test