[clang][Analyzer] Move checker 'alpha.unix.Errno' to 'unix.Errno'.

[balazske]

No description provided.

[balazske]

This checker was dependent on unix.StdCLibraryFunctions. After that checker was moved out of alpha, it is possible to move alpha.unix.Errno out of alpha.
The checker was tested first on the following projects. The following links were automatically generated to compare the checker results before and after the change. What matters only is the "new reports" where the reports from the checker are shown with the new name (and in the "lost reports" with the old name), there should be no other difference. I did not find obvious false positives, and there are not many results. Most interesting are the cases found in postgres, even these do not look as false positive. (The checker assumes that functions like fwrite may write 0 bytes and return 0 if the number to write is 1, and this is different from failure of the function when it returns -1.)

memcached_1.6.8_errno_alpha_unix_vs_memcached_1.6.8_errno_unix New reports Lost reports
tmux_2.6_errno_alpha_unix_vs_tmux_2.6_errno_unix New reports Lost reports
curl_curl-7_66_0_errno_alpha_unix_vs_curl_curl-7_66_0_errno_unix New reports Lost reports
twin_v0.8.1_errno_alpha_unix_vs_twin_v0.8.1_errno_unix New reports Lost reports
vim_v8.2.1920_errno_alpha_unix_vs_vim_v8.2.1920_errno_unix New reports Lost reports
openssl_openssl-3.0.0-alpha7_errno_alpha_unix_vs_openssl_openssl-3.0.0-alpha7_errno_unix New reports Lost reports
sqlite_version-3.33.0_errno_alpha_unix_vs_sqlite_version-3.33.0_errno_unix New reports Lost reports
ffmpeg_n4.3.1_errno_alpha_unix_vs_ffmpeg_n4.3.1_errno_unix New reports Lost reports
postgres_REL_13_0_errno_alpha_unix_vs_postgres_REL_13_0_errno_unix New reports Lost reports

[NagyDonat]

If this checker doesn't do anything without unix.StdCLibraryFunctions, then this should be formalized as a dependency relationship (let this depend on unix.StdCLibraryFunctions). Are there any technical obstacles?

[balazske]

The dependency is only to have modeling checkers that are dependencies of other (non-modeling) checkers. Weak dependency is to have a fixed order of the checkers. Only a new type of dependency could work for this case. (The problem is that StdCLibraryFunctionsChecker is both a modeling and non-modeling checker. The situation can be improved only with bigger changes.)

[NagyDonat]

Thanks for the clarification. I think it'd be important to improve this situation (it's very user-hostile to hide an "if you want to use this, you must manually enable another checker" remark in the docs), but this is independent of the currently reviewed commit.

[balazske]

The checker was tested additionally on projects libwebm, bitcoin, contour and produced no results.

[NagyDonat]

This report from vim seems to be a false positive:

Readlink returns either -1 (failure, errno is set) or the number of characters read from the symlink entry (i.e. the filename the symlink points to). I don't think that it's possible to create a "symlink to emptystring", so the the checkers should recognize that if the return value is <= 0, then it's a failure.

[NagyDonat]

I also think that the reports from postgres that you mentioned are too confusing. They follow this pattern:

The function fwrite() is declared as

size_t fwrite(const void *restrict ptr, size_t size, size_t nitems,
           FILE *restrict stream);

and it usually indicates success by returning the value of nitems -- but it has a corner case that if size is 0, then it does nothing and returns 0. This corner case is considered to be a successful call, and the bug reports are generated when the analyzer follows this case and notices that errno contains an undefined value.

I think that in these reports it would be important to replace the generic after successful call to 'fwrite' with a concrete message to highlight that the analyzer is following this obscure corner case (which is sometimes missing from the documentation, for example on my system man 3 fwrite from release 5.05 of the Linux man-pages project does not mention it).

[NagyDonat]

I'd say that the readlink modeling tweak is not a blocking issue; but the bug reports coming from "size is zero" corner case should be clarified (in a separate commit) before merging this.

As these are both easy to fix, I hope that we can move this checker out of alpha soon.

[balazske]

PRs #71373 and #71392 are created to improve the indicated problems.

[balazske]

I also think that the reports from postgres that you mentioned are too confusing.

After the change in #71392 the report looks like this. This looks somewhat better, probably still false positive because the "len" can not be 0, but the checker does not have more information.

[NagyDonat]

I also think that the reports from postgres that you mentioned are too confusing.

After the change in #71392 the report looks like this. This looks somewhat better, probably still false positive because the "len" can not be 0, but the checker does not have more information.

Thanks for the update!

I think these improvements are sufficient to move this checker out of alpha.

The new message which clarifies that the report is produced in the case "Assuming that argument 'size' to 'fwrite' is 0; 'errno' becomes undefined after the call" is completely sufficient to orient the user; and although the report is (arguably) a false positive, the user can quickly understand and discard it.

(This false positive is not too common and it's caused by the general limitations of the CSA engine, so it should not keep this checker in alpha state.)

@steakhal or anybody else interested:
Is there any objection to merging #71392 (the commit that clarifies the note tags) and this commit?