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

gamesh411 abandoned this revision. gamesh411 added a comment. 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

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

NoQ added a comment. 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

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

aaron.ballman added a reviewer: NoQ. aaron.ballman added a subscriber: NoQ. aaron.ballman added a comment. Herald added a subscriber: Charusso. In D83717#2366598 , @gamesh411 wrote: >> Just to make sure we're on the same page -- the current approach is

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

gamesh411 added a comment. > 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

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

aaron.ballman added a comment. In D83717#2364187 , @gamesh411 wrote: > 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

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

gamesh411 added a comment. In D83717#2279263 , @aaron.ballman wrote: > One of the concerns I have with this not being a flow-sensitive check is that > most of the bad situations are not going to be caught by the clang-tidy > version of the check. The

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

aaron.ballman added a comment. One of the concerns I have with this not being a flow-sensitive check is that most of the bad situations are not going to be caught by the clang-tidy version of the check. The CERT rules show contrived code examples, but the more frequent issue looks like:

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

gamesh411 updated this revision to Diff 292488. gamesh411 added a comment. Update commit message Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

gamesh411 updated this revision to Diff 292419. gamesh411 added a comment. Reformat diagnostic message Use explicit name longjmp instead of jump function Fix liberal auto inside Collector Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/

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

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

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

gamesh411 updated this revision to Diff 292416. gamesh411 marked 2 inline comments as done. gamesh411 added a comment. Add abort and terminate handling Extend tests to cover every exit functions Extract matcher bind labels Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:37 +return false; + constexpr StringRef ExitFunctions[] = {"_Exit", "exit", "quick_exit"}; + return llvm::is_contained(ExitFunctions, FD->getName());

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

gamesh411 updated this revision to Diff 289597. gamesh411 added a comment. only consider global and ::std scope handlers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64 +/// argument. +void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) { + const auto IsRegisterFunction = whisperity wrote: > aaron.ballman wrote:

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64 +/// argument. +void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) { + const auto IsRegisterFunction = aaron.ballman wrote: > whisperity wrote: > >

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:33 +bool isExitFunction(StringRef FnName) { + return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit"; +} njames93 wrote: > whisperity wrote:

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

gamesh411 updated this revision to Diff 282837. gamesh411 marked an inline comment as done. gamesh411 added a comment. rename file name in header Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:33 +bool isExitFunction(StringRef FnName) { + return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit"; +} whisperity wrote: > aaron.ballman wrote:

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:33 +bool isExitFunction(StringRef FnName) { + return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit"; +} aaron.ballman wrote: > Because this

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:33 +bool isExitFunction(StringRef FnName) { + return FnName == "_Exit" || FnName == "exit" || FnName == "quick_exit"; +} Because this rule applies in C++ as

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

whisperity added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:64 +/// argument. +void ExitHandlerCheck::registerMatchers(MatchFinder *Finder) { + const auto IsRegisterFunction = What happens if this test is run on C++ code

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

gamesh411 updated this revision to Diff 282535. gamesh411 added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

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

gamesh411 added a comment. I have updated the revision to use `llvm::move` algorithm (available thanks to @njames93). Friendly ping :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717

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

gamesh411 updated this revision to Diff 282533. gamesh411 added a comment. ensure lint in path Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

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

gamesh411 updated this revision to Diff 282532. gamesh411 added a comment. use llvm::move algorithm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

gamesh411 updated this revision to Diff 282529. gamesh411 added a comment. use llvm::move algorithm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

njames93 added a comment. In D83717#2155103 , @gamesh411 wrote: > My thoughts exactly! I also thought about anchor-points as a feature in > file-check, as that would immensely increase the readability of the test-code > in such cases. I've put in an

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

gamesh411 added a comment. In D83717#2155066 , @njames93 wrote: > In D83717#2152762 , @gamesh411 wrote: > > > In D83717#2150977 , @njames93 > > wrote: > > > > >

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

njames93 added a comment. In D83717#2152762 , @gamesh411 wrote: > In D83717#2150977 , @njames93 wrote: > > > Alternatively you could do something like this, though it would be a pain > >

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

gamesh411 marked 2 inline comments as done. gamesh411 added a comment. In D83717#2153246 , @Eugene.Zelenko wrote: > In D83717#2153245 , @gamesh411 wrote: > > > @Eugene.Zelenko I have just rebase-d, and seen that

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

Eugene.Zelenko added a comment. 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

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

gamesh411 added a comment. @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

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

gamesh411 updated this revision to Diff 278184. gamesh411 added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

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

gamesh411 updated this revision to Diff 278181. gamesh411 added a comment. tidy up release notes, make all new check entries uniform Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87 + ` check. + Finds functions registered by ``atexit`` and ``at_quick_exit`` that are calling + exit functions ``_Exit``, ``exit``, ``quick_exit`` or ``longjmp``.

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

gamesh411 added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87 + ` check. + Finds functions registered by ``atexit`` and ``at_quick_exit`` that are calling + exit functions ``_Exit``, ``exit``, ``quick_exit`` or ``longjmp``.

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

gamesh411 updated this revision to Diff 278178. gamesh411 marked an inline comment as done. gamesh411 added a comment. add missing newline in release notes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87 + ` check. + Finds functions registered by ``atexit`` and ``at_quick_exit`` that are calling + exit functions ``_Exit``, ``exit``, ``quick_exit`` or ``longjmp``.

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

gamesh411 updated this revision to Diff 278176. gamesh411 marked an inline comment as done. gamesh411 added a comment. . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

gamesh411 updated this revision to Diff 278174. gamesh411 added a comment. fix documentation header Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

gamesh411 marked 2 inline comments as done. gamesh411 added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:6 + +This check implements SEI CERT rule ENV32-C by finding functions registered by +``atexit`` and ``at_quick_exit`` that are

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst:6 + +This check implements SEI CERT rule ENV32-C by finding functions registered by +``atexit`` and ``at_quick_exit`` that are calling exit functions ``_Exit``,

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

gamesh411 added a comment. @steakhal @Eugene.Zelenko Thanks for checking this patch! I have tried my best to adhere to your suggestions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717

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

gamesh411 marked an inline comment as done. gamesh411 added a comment. In D83717#2150977 , @njames93 wrote: > Alternatively you could do something like this, though it would be a pain >

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

gamesh411 marked 12 inline comments as done. gamesh411 added inline comments. Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:132 + "terminate by returning"); + break; +} steakhal wrote: > Why don't we `return` here? > Same

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

gamesh411 updated this revision to Diff 278116. gamesh411 marked 2 inline comments as done. gamesh411 added a comment. use move instead of copy fix documentation issues fix tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/

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

njames93 added a comment. 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 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

njames93 added a comment. In D83717#2150099 , @gamesh411 wrote: > extend with notes > apply minor fixes > tests are WIP until I figure out how to properly use file-check If you add tests for notes, you will need to use `CHECK-MESSAGES-DAG` and

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

Szelethus added reviewers: Szelethus, martong, steakhal, dkrupp. Szelethus added a comment. @Eugene.Zelenko Thanks for cleaning up revisions -- same as D69560#1725399 , we are working in the same office and have worked on some forms of static analysis

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:87 + ` check. + Finds functions registered by ``atexit`` and ``at_quick_exit`` that are calling + exit functions ``_Exit``, ``exit``, ``quick_exit`` or ``longjmp``.

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

steakhal added a comment. Unfortunately, I'm not qualified enough to have much to say. Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:1 +//===--- Env32CCheck.cpp - clang-tidy -===// +// Env32CCheck.cpp

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

gamesh411 marked 10 inline comments as done. gamesh411 added a comment. 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. Comment at:

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

gamesh411 updated this revision to Diff 277796. gamesh411 added a comment. extend with notes apply minor fixes tests are WIP until I figure out how to properly use file-check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/

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

njames93 added a comment. It would be a good idea to emit notes to help explain the problem code, I'm thinking something along the lines of: warning: exit-handler potentially calls a jump function. Handlers should terminate by returning [cert-env32-c] note: exit-handler declared here

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

gamesh411 updated this revision to Diff 277590. gamesh411 added a comment. mention iterator header Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

gamesh411 updated this revision to Diff 277588. gamesh411 added a comment. use std::copy algorithm polish Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

gamesh411 updated this revision to Diff 277581. gamesh411 added a comment. fix test diag location changes resulting from autoformatting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

gamesh411 updated this revision to Diff 277575. gamesh411 added a comment. simplify VisitCallExpr Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files:

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

gamesh411 updated this revision to Diff 277571. gamesh411 added a comment. update includes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83717/new/ https://reviews.llvm.org/D83717 Files: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp

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

gamesh411 created this revision. Herald added subscribers: llvm-commits, cfe-commits, martong, steakhal, Szelethus, dkrupp, xazax.hun, whisperity, mgorny. Herald added projects: clang, LLVM. Add check for the SEI CERT item ENV32-C which dictates that exit handler functions should terminate by