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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
