[PATCH] D135360: [clang][analyzer] Add some more functions to StreamChecker and StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D135360#3981420 <https://reviews.llvm.org/D135360#3981420>, @balazske wrote:

> My current approach is that the POSIX is more strict than the C standard (POSIX allows a subset of what C allows). I do not see (errno related) contradiction between these standards

Alright, so if we wanna support the C standard thinking, we would just create an option that would be a little more noisy. Sounds fair enough.

> (except initialization to zero of errno missing from POSIX, and possible negative value in errno in POSIX). One problem exists now, that `errno` is set to a non-zero (but not positive only) value at error. Probably we should change this to be always positive at error, according to the C standard `errno` is positive only, and POSIX does not tell that errno can be negative.

According to this (which I assume you've seen as well): https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html

> Description
> ===========
>
> [...]
> Some of the functionality described on this reference page extends the ISO C standard. Any conflict between the requirements described here and the ISO C standard is unintentional. [...]
> [...]
>
> Issue 6
> =======
>
> Values for errno are now required to be distinct positive values rather than non-zero values. This change is for alignment with the ISO/IEC 9899:1999 standard.

So yes, I'd say its reasonable to assume in POSIX modthat if `errno` is non-zero, it is **positive**.

> The checker currently works only in POSIX mode for every function, the **ModelPOSIX** setting controls only if additional functions are added (all with POSIX rules) (these functions could be added with C rules too).

I see a lot of mention of POSIX vs. the C standard, yet there seems to be no mention of these differences in the comments added by this patch or already commited. Maybe we could do better on documenting this important difference in behaviour.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1818-1857
+    // int fgetpos(FILE *restrict stream, fpos_t *restrict pos);
+    // From 'The Open Group Base Specifications Issue 7, 2018 edition':
+    // "The fgetpos() function shall not change the setting of errno if
+    // successful."
+    addToFunctionSummaryMap(
+        "fgetpos",
+        Signature(ArgTypes{FilePtrRestrictTy, FPosTPtrRestrictTy},
----------------
martong wrote:
> It is very good to see these new summaries! Would they be meaningful and usable without the changes in the StreamChecker? If yes, then I think we should split this into 2 patches.
Can you please do this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135360/new/

https://reviews.llvm.org/D135360