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

Wed Jun 24 09:43:29 PDT 2020

Szelethus added a comment.

I went through this a lot more thoroughly, as well as the previous patch, and this looks great, especially for an alpha option. I will admit, I'm a bit concerned by the lack of individual tests (what if a stupid interaction with another checker causes issues?), but I feel the need for a scale-able solution and your debug functions serve to help on this. If you don't mind, before I formally accept, it would be great to talk this over in a meeting and/or on cfe-dev, just so that I'm a bit more in the clear on what the direction is.

In D82288#2111759 <https://reviews.llvm.org/D82288#2111759>, @martong wrote:

> > None of what I said is a problem introduced by this specific patch
>
> Well, perhaps we should have had this conversation in a form of an RFC in cfe-dev instead. Next time maybe. Now, I am afraid that the review of the concrete changes in this patch are being neglected because the focus may be shifted here on the `evalCall` returns `true` problem (which does not happen in this patch).

Sure, good point, its just that this is the patch in my eye that really grows the impact of this checker (and transitively, the scope of the evalCall problem) above a threshold, which is why my concern was voiced here. But you're totally right, important discussions on design that is outside the scope of a single checker should have more visibility.

> But maybe a different naming would be needed: what about `EvalCallHereAndInvalidateErrno`?
>  Since all of the functions here are not pure - they have `NoEvalCall` set -, probably we should have just simply `InvalidateErrno` rather, what do you think? I agree, that this checker is a good place to invalidate errno where applicable, but If you don't mind I'd rather do that in a subsequent patch to keep the incremental approach.

Either is good, and it would be better in a followup patch indeed.

> Yes, there are many standard functions that are modeled in different checkers. Ideally all functions would be modeled in just one checker, I agree.

Thats not the point I wanted to get across -- its fine if multiple checkers model different aspects of the same function, but it should be done consistently, which at the moment isn't. It would be nice to have an agreement (even if that later changes) on where we see function modeling when this checker is a bit more mature. That also deserves its own discussion on cfe-dev.

> [...] Sooner or later `popen` would be modeled in `StreamChecker` and then we may remove the summary from here. But if we leave it here that would not be a problem either. I consider this checker as a generic checker and `StreamChecker` or `CStringModeling` as a specialized checker. The specialized checkers should be a weak dependency to this generic checker, so any bug related to e.g. `popen` is prioritized to `StreamChecker`. That's exactly what we did in D79420 <https://reviews.llvm.org/D79420>.

Or, numerical constraints would be checked here, the mentioned checkers would //strongly// depend on this one. That way, ranges and nullability would be focused here, and more specific functionalities in their respective checkers.

>> I didn't read this patch line-by-line, but I trust that its okay if Endre did so.
> 
> Well, okay :) But, at least //one// body should go through each line, otherwise how can we make sure that I don't inject some blunder in any of those lines? I don't expect you to check the specification of each function in the POSIX standard, just to have at least a quick look whether the argument constraints match the signature of the functions.

Sure! I would accept if I didn't have confirmation that someone did do that! :) Just wanted to get the high level stuff out of the way.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82288