[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. As I don't use this checker, thus I cannot speak of my experience. However, the reasons look solid and I'm fine with moving this checker to `unix.StdCLibraryFunctions`. Let some time for

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske added a comment. About the questions: - How many issues does it raise? Would we flood the user? I did not experience that the checker produces many warnings. Any warning from this checker is connected to a function call of a standard API, and the number of such calls is usually not

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

NoQ added a comment. It doesn't have to be in `core` just because it's dealing with "core features" of the language. The `core` package is for checks without which path-sensitive analysis becomes //so incredibly incorrect// that we don't want to support such configuration, we don't want our

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

steakhal added a comment. Additionally, it might make sense to first "release" the checker, and after another llvm release, turn this into a Core checker. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152436/new/ https://reviews.llvm.org/D152436

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

steakhal added a comment. Thanks for the sample reports. I'm fine if we want to make it a non-alpha (released) checker. An orthogonal question is, whether we want to have it under the code package. I'm not sure there are official guidance for elevating a checker to code, but here are my

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske added a comment. I have checked the results on some projects (memcached,tmux,curl,twin,vim,openssl,sqlite,ffmpeg,postgres,xerces,bitcoin). These results are more interesting, some look correct, some probably not:

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske added a comment. This checker was originally in the "unix" package. I agree that this is not exact and `core` can be better, the checked functions should be available in any default C library on UNIX, OSX, Windows or other platforms too, even the POSIX ones at least in some cases.

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

donat.nagy added a comment. Why is this checker placed in the "unix" group (instead of e.g. "core")? As far as I understand it can check some POSIX-specific functions with a flag (that's off by default) but the core functionality checks platform-independent standard library functions.

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske updated this revision to Diff 54. balazske added a comment. Herald added a subscriber: wangpc. Using the latest version of the checker. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152436/new/ https://reviews.llvm.org/D152436 Files:

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

Szelethus added a comment. In D152436#4443858 , @balazske wrote: > In D152436#4438956 , @NoQ wrote: > >> I'm somewhat skeptical of the decision made in D151225 >> because the

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske added a comment. In D152436#4438956 , @NoQ wrote: > Uh-oh, looks like I'm not paying nearly enough attention to this discussion > (sorry about that!!) > > I'm somewhat skeptical of the decision made in D151225 >

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske added a comment. For first experiment I have made patch D153612 that adds a `NoteTag` to "all" standard function calls. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152436/new/

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

NoQ added a comment. > In this case function `fileno` returns -1 because of failure, but this is not > indicated in a `NoteTag`. This is a correct result, only the note is missing. > This problem can be solved if a note is displayed on every branch ("case") of > the standard C functions. But

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

NoQ added a comment. Uh-oh, looks like I'm not paying nearly enough attention to this discussion (sorry about that!!) I'm somewhat skeptical of the decision made in D151225 because the entire reason I originally implemented `StdCLibraryFunctions` was to deal

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

donat.nagy added a comment. In D152436#4437822 , @steakhal wrote: > In D152436#4437692 , @donat.nagy > wrote: > >> Personally I think it's completely acceptable if the analyzer sometimes >> emits bug reports

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

steakhal resigned from this revision. steakhal added a comment. I resign as a reviewer as I'm not deeply connected to this checker, thus I won't block it or accept it. However, my opinion is that a checker should be "released" if they have clear diagnostics (which includes that it doesn't flood

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

donat.nagy added a comment. In D152436#4432736 , @balazske wrote: > [...] > From these two solutions, which one is better? (Show many unnecessary notes, > or show only necessary ones but lose some of the useful notes too.) I think we must avoid

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

Szelethus added a comment. In D152436#4432736 , @balazske wrote: > From these two solutions, which one is better? (Show many unnecessary notes, > or show only necessary ones but lose some of the useful notes too.) How likely are the problems with the

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske added a comment. It is possible to add note tags to show decisions at standard functions. For example at `fileno` show if it has failed or not failed. The most simple way is to add it to all places, this means a note will show up on any bug path at all standard function usages. This

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

Szelethus added a comment. In D152436#4411912 , @steakhal wrote: > In D152436#4408811 , @balazske > wrote: > >> In D152436#4408301 , @steakhal >> wrote: >> >>> I looked

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

steakhal added a comment. In D152436#4408811 , @balazske wrote: > In D152436#4408301 , @steakhal > wrote: > >> I looked at the TPs, and if the violation was introduced by an assumption >> (instead of an

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske added a comment. In D152436#4408301 , @steakhal wrote: > In D152436#4405558 , @balazske > wrote: > >> These are reports that could be improved: >> link >>

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

steakhal added a comment. In D152436#4405558 , @balazske wrote: > These are reports that could be improved: > link >

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

Szelethus added a comment. > I am not sure about the exact requirements, this review can be a place for > discussion about what should be fixed (if any). D52984 added the "Making your checker better" section to the dev manual:

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske added a comment. Another question is if default value of `ModelPOSIX` can be changed to true? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152436/new/ https://reviews.llvm.org/D152436 ___

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske added a comment. One deficiency is that some filenames of test files contain the old **std-c-library-functions-arg** name that is not used any more. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152436/new/

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske added a comment. I could test the checker on these projects (CTU analysis was not used): memcached,tmux,curl,twin,vim,openssl,sqlite,ffmpeg,postgres,tinyxml2,libwebm,xerces,bitcoin,protobuf,qtbase,contour,acid These are reports that could be improved: link

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

balazske created this revision. Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Herald added a reviewer: Szelethus. Herald added a reviewer: NoQ. Herald added a