[PATCH] D72705: [analyzer] Added new checker 'alpha.unix.ErrorReturn'.

Tue Sep 29 09:59:31 PDT 2020

Szelethus added a comment.

In D72705#2274568 <https://reviews.llvm.org/D72705#2274568>, @balazske wrote:

> I am not sure if this checker is worth to further development. A part of the found bugs can be detected with other checkers too, specially if the preconditions of many standard functions are checked (more than what is done now) and by modeling the function calls with assumed error return value.

We shouldn't throw all the knowledge away, once its in our hands. What you're suggesting as an alternative is using state splits, something that is obviously expensive, and we can't know whether that state split is always just / constructive.

---

Since we had private meetings about this, it would be great to share the overall vision going forward. While we shouldn't burden @NoQ with having to review this checker line-by-line, he might have valuable input on what we're going for.

I'd like to ask you to write a mail on cfe-dev, and bring forward a formal proposal:

- Explain ERR33-C, and your interpretation of it. Highlight how you want to see very explicit error checking against the concrete return value in some cases, and not in others. Provide a list of functions where you think, and why you think that vigorous checking is appropriate, and a list for those where a more lenient one could be used.
- Explain why you picked symbolic execution to solve this problem.
- Explain how this problem could be solved, and the pros/cons of them:
  - Making state splits in StdLibraryFunctionChecker
  - Expanding on taint analysis
  - Check constraints on a return value in a new checker (this is impossible, sure, but why?)
  - GDM tracking of values to be checked in a new checker
- You picked no4 -- justify it.

Since you made considerable progress already, it would be great to have talk about how you are implementing this, and why. For instance, "I recognize appropriate function in checkPostCall, and mark their return value as unchecked in the GDM. Later, in checkLocation, if I find that such a value is read from, I'll analyze the surrounding code to find out whether its a rule-compliant comparison of it, if it is not, I emit a warning. This is done by ascending the AST with a ParentMap, and running a statement visitor on the parent statement". Code examples are always amazing, demonstrate on a piece of code how `if(someReturnValue != 0)`, or something similar would be analyzed.

---

I hope I didn't sounds condescending or demanding, it was definitely not my intent. Its a joy to see your works come to life, there are a lot of smarts to marvel in! With that said, the reviewing process has shown some significant problems.

D71510 <https://reviews.llvm.org/D71510> was submitted 10 months ago -- you obviously didn't work on it non-stop, but the fact that some pivotal aspects of your solution was realized by me only a month ago despite doing my best to follow the development is very unfortunate indeed. I'm not sure anyone else realized the extent of it either. Changes on StreamChecker followed a very similar trend, I felt like I had to understand, explain, and prove the correctness of the change for myself. As I made progress on it, sometimes the patches were split up, reuploaded and abandoned, like a multi-headed hydra. We ought to draw some lessons from this.

As an author, you can make the reviewers job a lot easier by:

- If the change is big enough, have a round on the mailing list //before// writing too much code. Scouting ahead is fine, but its not worth going too far.
- Provide a thorough summary with examples, drawings (D70411#1754356 <https://reviews.llvm.org/D70411#1754356>), whatever that helps to convey the message. If the code you're changing took considerable time to understand, it will probably take considerable time for reviewers as well. D86874 <https://reviews.llvm.org/D86874>, D55566 <https://reviews.llvm.org/D55566>, D86135 <https://reviews.llvm.org/D86135> and D86736 <https://reviews.llvm.org/D86736> are great examples. D63954 <https://reviews.llvm.org/D63954> keeps it short, but provides a link to more discussions.
- For a new checker, upload a very slimmed down version of your prototype. Its okay if it has FP/FN problems, or crashes on some weird input, its alpha for a reason. Its rare that for demonstration purposes you really need 500+ LOC. This allows everyone to discuss followup steps as well (though I don't think you ever left me in the dark regarding that).
- If a high level objection/question rose, its not worth adding new features, or even updating it much, unless it directly clarifies some trivial misunderstanding.
- When you publish results, give your input on them. Are you satisfied with the findings? Are there notable false positives? Could they be fixed easily?
- Laugh at how I used to do none of these, again, because its funny (D45532#1083670 <https://reviews.llvm.org/D45532#1083670>).

There are a lot of lessons to be taken on my side as well. I should've been able to realize this feature earlier, and I could've asked for more info earlier and with more persistence. I don't pair my objections with enough appreciation for the invested effort.

Lets try to get this right, or done better, on the next patch!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72705