[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

Wed Jun 21 08:49:28 PDT 2023

donat.nagy added a comment.

In D152436#4437822 <https://reviews.llvm.org/D152436#4437822>, @steakhal wrote:

> In D152436#4437692 <https://reviews.llvm.org/D152436#4437692>, @donat.nagy wrote:
>
>> Personally I think it's completely acceptable if the analyzer sometimes emits bug reports that are true positives but lack a message [...]
>
> I must admit that I'm in the other camp.

To clarify my position the "sometimes" in my comment should've been "rarely" or something closer to that. I don't want to release confusing garbage, I'm just trying to optimize sum(helpfulness of checkers) and I feel that the last few steps towards perfect helpfulness are often disproportionately expensive. If a code contains two bugs, then two rough but understandable warnings are more valuable than one well-polished friendly warning + one completely missed bug (because we didn't have time to write a checker for it).

---

@balazske:

Do I understand it correctly that the "create note that's shown when the return value is marked as interesting" (the option 2 that I suggested) adds / will add clarifying notes to the fileno/fstat error reports and the ftell issue <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=xerces_v3.2.3_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=4ab640064066880ac7031727869c92f4&report-id=1932564&report-filepath=%2aThreadTest.cpp>? Did you see any real-life examples where this option 2 does not provide useful notes? If not, then I think we can accept that option 2 is a sufficient solution for these issues.

In addition to this, there are these issues noted by @Szelethus:

In D152436#4408199 <https://reviews.llvm.org/D152436#4408199>, @Szelethus wrote:

> In D152436#4405558 <https://reviews.llvm.org/D152436#4405558>, @balazske wrote:
>
>> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=tmux_2.6_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=e7c81b8628bb636f9087a14b446dcb00&report-id=1930482&report-filepath=%2aclient.c>
>> The function `open` is not modeled in `StdCLibraryFunctionsChecker`, it should not return less than -1 but this information is not included now.
>> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=twin_v0.8.1_stdclibraryfunctions_test&is-unique=on&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=ed57f448095d9ba67d047a45898d1ebb&report-id=1930547&report-filepath=%2alibTw.c>
>> `socket` can not return less than -1 but this function is not modeled currently.
>
> These should be a rather painless fix, right?
>
> [...]
>
>> link <https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=postgres_REL_13_0_stdclibraryfunctions_test&is-unique=on&page=2&diff-type=New&checker-name=unix.StdCLibraryFunctions&report-hash=ebc6fce212f7238143e8f97b1d231abf&report-id=1932035&report-filepath=%2arelcache.c>
>> `fwrite` with 0 buffer and 0 size should not be an error, this is not checked now.
>
> Some discussion for that: D140387#inline-1360054 <https://reviews.llvm.org/D140387#inline-1360054>. There is a FIXME in the code for it -- not sure how common this specific issue is, but we did stumble on it in an open source project... how painful would it be to fix this?

If it's not too difficult, then please upload a new version of this patch that implements "option 2" (i.e. produces notes when the return value of the std library function is marked as interesting) + handles the three requests of @Szelethus. I hope that you can do this and then this review can be concluded quickly.

If there are significant obstacles (or I misunderstood the situation), then please reply and discuss the next steps.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152436