balazske added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:310 + const MemRegion *ErrnoR = State->get<ErrnoRegion>(); + if (!ErrnoR) + return State; ---------------- Szelethus wrote: > When can this occur? Maybe we can turn this early return to an assert -- when > `ModelPOSIX` is true, this mustn't be null (if what I'm saying is correct). I am not totally sure that `errno` is found always (definition of `errno` is encountered) if a summary for a standard function is added. The standard function in itself must be there, otherwise no summary is added and this function should not be called. But definition of `errno` may reside in other header (errno.h) that is probably not included in the translation unit. If the errno definition is not found the `ErrnoRegion` is not set (the checker may still turned be on but does nothing, but these functions are allowed to be called). Existence of `errno` is not even dependent on `ModelPOSIX`, `errno` is defined in the (non POSIX) C standard. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:839 + Result += DK == Violation ? " should not be zero" : " is not zero"; + return Result.c_str(); +} ---------------- Szelethus wrote: > Just thinking aloud, no changes required here necesserily -- in most places, > we use a combination of `SmallString` and `raw_svector_ostream` so construct > strings, but I can't find anything set in stone about the practice (mostly > looking at [[ > https://llvm.org/docs/ProgrammersManual.html#string-like-containers | LLVM's > Programmers Manual ]]). Isn't this just good enough? Is `llvm::Twine` worth > the hassle? The `Twine` may work is all partial strings are constant pointers, this is not true here. The `getArgDesc` does not return a constant string, the returned value must be copied. Still it may work if one single expression is used to construct the string. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1691-1702 + .Case({ArgumentCondition(1U, WithinRange, Range(1, SizeMax)), + ArgumentCondition(2U, WithinRange, Range(1, SizeMax)), + ReturnValueCondition(BO_LT, ArgNo(2)), ReturnValueCondition(WithinRange, Range(0, SizeMax))}, - ErrnoIrrelevant) + ErrnoNEZeroIrrelevant) + .Case({ArgumentCondition(1U, WithinRange, Range(1, SizeMax)), + ReturnValueCondition(BO_EQ, ArgNo(2)), ---------------- Szelethus wrote: > If I undetstand correctly, these 3 cases will cause the current path of > execution to split into 3. I vaguely recall some arguments against this, but > that's been a while, did the stance on this change? I see a number of already > commited summaries with 3 cases, so this could be fine for all I know. The problematic case is if the function is not evaluated by `StreamChecker`, otherwise `StreamChecker` already sets the constraints on the return value and argument and some of the cases here should become unsatisfied. It may be known about the argument 1 that it can not be zero (or is only zero), then it is only 2 possible branches. (Still it can happen often that all 3 branches are possible.) This split is needed to make the summary correct and not dependent on if `StreamChecker` evaluates the function or not. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1705-1707 + // FIXME: It should be allowed to have a null buffer if any of + // args 1 or 2 are zero. Remove NotNull check of arg 0, add a check + // for non-null buffer if non-zero size to BufferSizeConstraint? ---------------- Szelethus wrote: > Maybe `StreamChecker` could take over the handling of that case? Seems like > it also warns about the nullness of the first `fread` parameter. The current approach was that the NULL argument warning is generated by one checker only, `StreamChecker` or this checker. The NULL check is already built into these summaries so this is the better place for the warning. The NULL warning is removed from `StreamChecker` in D137790. Additionally the same problem can happen with `BufferSizeConstraint` at other function summaries, we can say generally (or more often) that if a buffer has size of 0 it is allowed to be NULL pointer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140387/new/ https://reviews.llvm.org/D140387 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
