[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. In D91000#4099773 , @dyung wrote: > To XFAIL the test, grep for lines with "XFAIL" and "ps4" and you should find > some examples. It was recently changed how it worked lately. Thank you, yes I found an example. I went with

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

dyung added a comment. In D91000#4099744 , @whisperity wrote: > In D91000#4099669 , @dyung wrote: > >> -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-scei-ps4 >> >> Hopefully this can help you to reproduce the problem. > >

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. In D91000#4099669 , @dyung wrote: > -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-scei-ps4 > > Hopefully this can help you to reproduce the problem. Ah, thank you very much. And indeed, giving `--target=x86_64-scei-ps4` to Clang-Tidy

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

dyung added a comment. In D91000#4099657 , @whisperity wrote: > Ping @dyung, it looks like you're the owner of the relevant build-bot. I > can't find any information what `x86_64-sie` is... > > Meanwhile, it seems my attempt at fix is not actually fixing

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a subscriber: dyung. whisperity added a comment. Ping @dyung, it looks like you're the owner of the relevant build-bot. I can't find any information what `x86_64-sie` is... Meanwhile, it seems my attempt at fix is not actually fixing anything, I'm trying to figure out how to

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. Alright, right now, I have no meaningful idea why this failure appears , locally I'm trying every test command as they appear in the test, and all the tests

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. Oddly enough, one of the buildbots caught a not matching test that was working locally... on it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91000/new/ https://reviews.llvm.org/D91000

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

This revision was automatically updated to reflect the committed changes. Closed by commit rGf27c8ac83e7c: [clang-tidy] Add the `bugprone-unsafe-functions` check (authored by futogergely, committed by whisperity). Changed prior to commit: https://reviews.llvm.org/D91000?vs=485774=494265#toc

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity accepted this revision. whisperity added a comment. This revision is now accepted and ready to land. Alright, I've done a full reanalysis after a rebase. Overhead is not meaningfully measurable, even for complex TUs such as LLVM. The check is viable in C++ code as it finds cases

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. Alright, the tests came back from the CI positively and there were no crashes on like 15 projects (about 50:50 C and C++). There are some nice results: - In LLVM, in `LexPPMacroExpansion.cpp` `ExpandBuiltinMacro()` there is a hit for a call `Result = asctime(TM);`.

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. Tentative **LGTM**, will still need to run the "usual test projects" in the CI for confirmation. What's missing from this patch are the `.rst` files for the CERT-specific aliases of the check, but those are trivial and we can "add them in post" anyway. CHANGES

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. In D91000#4020891 , @futogergely wrote: > What we could do is: > > 1. add a new checker option to decide if we suggest replacements from AnnexK. > We could avoid registering matchers this way, but I don't really like this, >

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

futogergely added a comment. In D91000#3862210 , @whisperity wrote: > In D91000#3861942 , @aaron.ballman > wrote: > >> My concern with that approach was that we pay the full expense of doing the >> matches only

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

futogergely updated this revision to Diff 485774. futogergely marked 5 inline comments as done. futogergely removed a reviewer: ktomi996. futogergely added a comment. Addressing review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91000/new/ https://reviews.llvm.org/D91000

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

futogergely added a comment. Hi, sorry for the late answer, did not have time to check this in the last few weeks. I will try to address all of the remaining comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91000/new/ https://reviews.llvm.org/D91000

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. In D91000#3861942 , @aaron.ballman wrote: > My concern with that approach was that we pay the full expense of doing the > matches only get get into the `check()` function to bail out on all the Annex > K functions, but now

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

aaron.ballman added a comment. In D91000#3770071 , @whisperity wrote: > Generally LGTM. Please revisit the documentation and let's fix the style, and > then we can land. > > In D91000#3197851 , @aaron.ballman >

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. In D91000#3770071 , @whisperity wrote: > In D91000#3197851 , @aaron.ballman > wrote: > >> In terms of whether we should expose the check in C++: I think we shouldn't. >> [...] >> >> I

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. Generally LGTM. Please revisit the documentation and let's fix the style, and then we can land. In D91000#3197851 , @aaron.ballman wrote: > In terms of whether we should expose the check in C++: I think we shouldn't. >

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

balazske added a comment. Another functions that can be added to the list: `atoi()`, `atol()`, `atoll()`, `atof()`. These are unsafe according to https://wiki.sei.cmu.edu/confluence/display/c/ERR34-C.+Detect+errors+when+converting+a+string+to+a+number. CHANGES SINCE LAST ACTION

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

futogergely updated this revision to Diff 440259. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91000/new/ https://reviews.llvm.org/D91000 Files: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:148 +- New :doc:`bugprone-unsafe-functions ` check. + Please keep checks entries in alphabetical order. Comment at:

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

futogergely updated this revision to Diff 440216. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91000/new/ https://reviews.llvm.org/D91000 Files: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

futogergely added a comment. In D91000#3612090 , @Eugene.Zelenko wrote: > Locations for tests and check documentation was changed recently. Please > rebase from `main` and adjust your code accordingly. Done. CHANGES SINCE LAST ACTION

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

Eugene.Zelenko added a comment. Locations for tests and check documentation was changed recently. Please rebase from `main` and adjust your code accordingly. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91000/new/ https://reviews.llvm.org/D91000

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

futogergely updated this revision to Diff 440202. futogergely added a comment. updates based on comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91000/new/ https://reviews.llvm.org/D91000 Files: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

futogergely marked 7 inline comments as done. futogergely added a comment. In D91000#3506605 , @whisperity wrote: > Just one question if you could try this out for me: what happens if you run > `clang-tidy a.c b.c` (two TUs in the invocation) where **one

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

whisperity added a comment. Just one question if you could try this out for me: what happens if you run `clang-tidy a.c b.c` (two TUs in the invocation) where **one of them** (preferably the later one, i.e. **`b.c`**) does //NOT// have Annex K enabled? I believe the cached `IsAnnexKAvailable`

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

futogergely updated this revision to Diff 421435. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91000/new/ https://reviews.llvm.org/D91000 Files: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

balazske added a comment. I found only small issues. Comment at: clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp:109 + + SourceRange Range = SourceRange(DeclRef->getBeginLoc(), DeclRef->getEndLoc()); + // FIXME: I'm not sure if this can ever happen, but to be

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

futogergely updated this revision to Diff 414359. futogergely retitled this revision from "[clang-tidy] Add cert-msc24-msc33-c checker." to "[clang-tidy] Add bugprone-unsafe-functions checker.". futogergely added a comment. Checker has been moved to bugprone. CHANGES SINCE LAST ACTION