martong marked 6 inline comments as done.
martong added inline comments.
Herald added a subscriber: ASDenysPetrov.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:175
+ using ValueConstraint::ValueConstraint;
+ bool CannotBeNull = true;
+
----------------
Szelethus wrote:
> What does this do? Is it ever used in the patch?
Yes, it is used. We use it in `apply` the value is passed to `assume`.
And in `negate` we flip the value.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:189
+ ValueConstraintPtr negate() const override {
+ NotNullConstraint tmp(*this);
+ tmp.CannotBeNull = !this->CannotBeNull;
----------------
balazske wrote:
> Is `Tmp` better name?
Absolutely, yes, I should avoid lower case variables, will rename.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:684
+ .Case({
+ ReturnValueCondition(LessThanOrEq, ArgNo(2)),
+ })
----------------
steakhal wrote:
> Two lines below you are using the `{0U}` initialization syntax, and here the
> simple constructor call syntax.
> Shouldn't we pick one?
Yes, definitely. I think I am going to use brace initialization syntax
everywhere.
================
Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints.c:59
+ fread(buf, sizeof(int), 10, fp); // expected-warning{{Function argument
constraint is not satisfied}}
+}
----------------
balazske wrote:
> One difficulty is that `fread` can work if the `buf` is 0 and `count` is 0
> too. There is for sure code that uses this feature.
My understanding is that in case of `fread`, if `buf` is 0 then that is an
undefined behavior.
In the section in C11 describing the use of library functions (§7.1.4) it is
stated that:
> If an argument to a function has an invalid value (such as a value outside
> the domain of the function, or a pointer outside the address space of the
> program, or a null pointer, or a pointer to non-modifiable storage when the
> corresponding parameter is not const-qualified) or a type (after promotion)
> not expected by a function with variable number of arguments, the behavior is
> undefined. If a function argument is described as being an array, the pointer
> actually passed to the function shall have a value such that all address
> computations and accesses to objects (that would be valid if the pointer did
> point to the first element of such an array) are in fact valid.
I've found more details in this StackOverflow post:
https://stackoverflow.com/questions/51779960/is-it-safe-to-fread-0-bytes-into-a-null-pointer
Also note that a sane libc implementation would not dereference `buf` if `size`
is 0, but we may not rely on these implementation details, that is why this is
declared as undefined behavior in the standard.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75063/new/
https://reviews.llvm.org/D75063
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
