Szelethus added a comment. Would be possible to test the errno specific changes as well?
================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:310 + const MemRegion *ErrnoR = State->get<ErrnoRegion>(); + if (!ErrnoR) + return State; ---------------- 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). ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:839 + Result += DK == Violation ? " should not be zero" : " is not zero"; + return Result.c_str(); +} ---------------- 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? ================ 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)), ---------------- 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. ================ 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? ---------------- Maybe `StreamChecker` could take over the handling of that case? Seems like it also warns about the nullness of the first `fread` parameter. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1848-1849 + // int fseek(FILE *stream, long offset, int whence); + // FIXME: It is possible to get the 'SEEK_' values (like EOFv) for arg 2 + // condition. + addToFunctionSummaryMap( ---------------- Sure, but what should be fixed? We should check whether the the last argument is a `SEEK_*` value? 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
