[PATCH] D82288: [analyzer][StdLibraryFunctionsChecker] Add POSIX file handling functions

Wed Jun 24 08:04:22 PDT 2020

gamesh411 accepted this revision.
gamesh411 added a comment.
This revision is now accepted and ready to land.

In D82288#2111206 <https://reviews.llvm.org/D82288#2111206>, @Szelethus wrote:

> I see a lot of `NoEvalCall`, but I wonder whether modifying `errno` warrants this. Shouldn't we have a alongside `NoEvalCall` and `EvalCallAsPure` an `EvalCallAsPureButInvalidateErrno` invalidation kind?
>
> Also, I'm kind of worried by this checker taking on the responsibility of modeling functions by `EvalCall`, except for a few functions that it also models, but not with `EvalCall`, it feels clunky. I remember this one time when I wanted to model `alloca()` <https://reviews.llvm.org/D68165?id=222257#inline-612891>, which has a `__builtin_alloca` variant as well, and I spent a good couple hours figuring out that why the descpription `{CDF_MaybeBuiltin, "alloca", 1}` was causing crashes. Turns out that `BuiltinFunctionsChecker` `EvalCall`ed the builtin version, but not the other.
>
> In general, I don't think we have a well established stance on whether we use `EvalCall`, or `PreCall`+`PostCall`, and what roles do batch modeling checkers like `StdLibraryFunctionsChecker`, `GenericTaintChecker` and `BuiltinFunctionsChecker` should play alongside mode sophisticated modeling checkers like `CStringModeling`, `DynamicMemoryModeling` and `StreamChecker`. I bet some of us have ideas how this is supposed to be done, but I feel like there is no unified, agreed upon road we're moving towards. This problem came up in D69662 <https://reviews.llvm.org/D69662> as well. D75612 <https://reviews.llvm.org/D75612> introduces `EvalCall`s to `StreamChecker`, and I was puzzled why this hasn't been a thing in all similar chekers like `MallocChecker`, which I commented on in D81315#2079457 <https://reviews.llvm.org/D81315#2079457>.
>
> It would also be great if we could enforce that no two checkers evaluate the same function.
>
> None of what I said is a problem introduced by this specific patch, but it definitely amplifies it with the modeling of functions such as `strncasecmp` (should be the responsibility of `CStringModeling`), `strdup` (`MallocChecker`) and `popen` (`StreamChecker`). I'd like to see something set in stone, such as what you're eluding to in D75612#inline-690087 <https://reviews.llvm.org/D75612#inline-690087>, that the role of this checker is to model **numerical** (null-ness, ranges, etc) pre- and post conditions for standard functions, nothing less, nothing more, and it is technically an error to reimplement this in other checkers. If that is the goal, I guess we have to implement tests for individual functions added in this patch to make sure that these conditions aren't already checked elsewhere.
>
> I didn't read this patch line-by-line, but I trust that its okay if Endre did so.

I find this piece of information extremely valuable.
This description about the state we are in right now should prove very educational to whoever embarking on getting the evalCall business done.

I tried to match every signature to the corresponding constraints, and as I found no glaring errors, I will accept this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82288