Szelethus added a comment. 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. ================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:352-357 CmdLineOption<Boolean, "DisplayLoadedSummaries", "If set to true, the checker displays the found summaries " "for the given translation unit.", "false", + Released>, ---------------- D78118#inline-723679 Please mark this `Hidden`. ================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:358-363 + CmdLineOption<Boolean, + "ModelPOSIX", + "If set to true, the checker models functions from the " + "POSIX standard.", + "false", Released> ---------------- I'm fine with not hiding this, but as a user, why would I never not enable it? If the reasoning is that this isn't ready quite yet, change `Released` to `InAlpha`. Otherwise, either mark this `Hidden` or give a user friendly explanation as to what is there to gain/lose by tinkering with this option. 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
