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

[Kristóf Umann via Phabricator via cfe-commits]

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