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

Wed Jun 24 09:09:49 PDT 2020

martong added a comment.

Thanks for the review guys!
I understand there are concerns about `evalCall`, so I am trying to dissolve those concerns below. :)

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.

Maybe there is a misunderstanding here. Using `NoEvalCall` results that the function may be evaluated conservatively or evaluated by any other checker:

  case NoEvalCall:
    // Summary tells us to avoid performing eval::Call. The function is possibly
    // evaluated by another checker, or evaluated conservatively.
    return false;

In my understanding, it means that errno could be invalidated by any other checker. Consequently, these changes made here are not affecting any existing modeling of errno. (We use NoEvalCall in all functions in this patch.)

> Shouldn't we have a alongside `NoEvalCall` and `EvalCallAsPure` an `EvalCallAsPureButInvalidateErrno` invalidation kind?

I'd rather not confuse the terms here. If a function is pure that means it does not have any side effects, i.e. it must not modify the global errno either. On the other hand, I see your point, and this is a good point. 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.

> It would also be great if we could enforce that no two checkers evaluate the same function.

Yes, that would be great, but I don't see any clear solution to achieve that right now, this is really challenging. One very immature idea: Maybe we could have a special option in the engine that is used only in testing and which would let the evaluation to go further even if a checker returned `true` in `evalCall`. And if we find a second event like that then we could assert.

> 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).

> ... 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.

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

One goals is to add argument constraints to functions in the C, C++ and POSIX standard. Some of these argument constraints are indeed handled in e.g. `CStringModeling`. However, it is very easy to gather these argument constraints and apply them massively. By doing this, the analysis can be more precise, so, I see a lot of gain here. 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>.

The other very important goal would be to make it possible to add argument constraints (or branches/cases) to //any// library functions. These libraries may never be modeled in the analyzer. In this case the problem is non-existent. Perhaps we should divide the checker to two different checkers once we reach that point.

> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82288