Add support for functions 'fgetpos', 'fsetpos', 'ftell', 'rewind'.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I found some anomalies during development:
- If the checker StdCLibraryFunctions is added as dependency for alpha.unix.Stream in checkers.td I get some "unexplainable" test failures.
- In the comments section at CERT ERR30-C is pointed out that function ftell can change the value of errno if it is successful. This is the "normal" expected behavior of all standard functions unless the documentation tells other. But this place (search for ftell) tells that ftell should not change errno if successful. The persons commented at the CERT rule probably checked only one or more of the C standards. But there should be no conflict between POSIX and C ("Any conflict between the requirements described here and the ISO C standard is unintentional."). It would be unconventional if different check rules are needed (and add of a global analyzer option for POSIX or C mode).
Could you please elaborate? I don't see how to help you with it without seeing more details.
- In the comments section at CERT ERR30-C is pointed out that function ftell can change the value of errno if it is successful. This is the "normal" expected behavior of all standard functions unless the documentation tells other. But this place (search for ftell) tells that ftell should not change errno if successful. The persons commented at the CERT rule probably checked only one or more of the C standards. But there should be no conflict between POSIX and C ("Any conflict between the requirements described here and the ISO C standard is unintentional."). It would be unconventional if different check rules are needed (and add of a global analyzer option for POSIX or C mode).
When it is ambiguous then I'd check the latest standards of both POSIX and C. If it is still doubtful then I'd vote for the C standard and would report a defect towards the POSIX community.
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1847–1886 | 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. |
If we look only at the C standard we can not tell much about if the functions should set errno. It seems that setting errno is totally implementation-dependent, except for a few functions that mention errno. These are fgetpos, fsetpos, ftell and should set errno to a positive value on error (but there is no guarantee that value of errno is preserved if no error occurs). And errno is always positive. This is different than what the checker currently does (with the new patches). Probably this should be a discourse question?
Okay then, I think it is worth to have a discourse question. But you could ask the wider "Clang" community, so I would not post the question as something that is related strictly to the static analyzer.
About the "ftell" problem: The POSIX rules are really an extension of the C standard rules. At ftell according to C standard errno should be set to a positive value if error occurs. The POSIX rules extend this: errno is not changed if no error occurs. This can be correct, then the CERT ftell non-compliant example is wrong (on success errno remains always 0). It can be fixed if line errno=0 is removed from the start of the non-compliant code. It may be best to use the POSIX rules for the checker, because the C standard does not say much and may need to require setting of errno to 0 before a standard function call.
Added more functions to StdLibraryFunctionsChecker.
This is redundant with the StreamChecker for now but
makes possible to remove the NULL stream check from
StreamChecker and reduce overlap of the checkers.
This way no dependency should be needed between these checkers.
Mind that dependencies also establish the order of callbacks (dependent checkers are called after their dependencies).
When it is ambiguous then I'd check the latest standards of both POSIX and C. If it is still doubtful then I'd vote for the C standard and would report a defect towards the POSIX community.
Interesting pair of perspectives, I think a reasonable checker should be a little more conservative, more akin to what POSIX seems to specify.
Did this thread ever materialize? I admit I didn't follow :)
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 (except initialization to zero of errno missing from POSIX, and possible negative value in errno in POSIX). 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). 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.
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.
I suppose the answer is no then :)
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1847–1886 | Can you please do this? |
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp | ||
---|---|---|
1847–1886 | This is done now. There were 2 consecutive patches that modify StdLibraryFunctionsChecker and other checkers. These are rearranged into 2 new patches. One for all changes in StdLibraryFunctionsChecker (and related changes in ErrnoModeling): D140387. Other for all other changes, StreamChecker and many of the tests that exercise both checkers: D140395. |
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.