[PATCH] D77150: [Analyzer] New Option for ContainerModeling: AggressiveEraseModeling

Mon Jul 13 05:44:01 PDT 2020

Szelethus added a comment.

There is a parallel discussion in this patch on how we should cater to user requests, I wrote a lengthy comment about my opinion, but I just don't think it adds much -- at the end of the day, it is fair that if we implement an feature for a small subset of users that is known to cause unpleasant side effects, we shouldn't make it a part of the default user interface, because among many other things it inevitably forces other contributors to maintain it, even if they weren't enthusiastic about the idea, but its also fair to prioritize direct users within reason.

Since this is in the same bullpark as D32905 <https://reviews.llvm.org/D32905>, I think this should land in some way or another.

> (@baloghadamsoftware) So you both are saying that I should tell the user that we cannot help, the community does not support that he catches this bug in early phase with the analyzer. He must catch it in the testing phase for higher costs. This is not the best way to build confidence in the analyzer. [...]

Nobody said we cannot help. What I say, is that we should not task the broad analyzer community with the maintenance of an option that is known to make state splits that we might not be untitled to. In fact,

> (@NoQ) [...] But you guys basically vend your own tool to your own customers and bear your own responsibility and i therefore can't tell you what to do in your realm, and i'm ok with having options that allow you to disagree with my choices.

I don't see how this option shouldn't be alpha -- it creates paths of execution that might be impossible. If I understand this correctly, we would emit a false positive here:

  std::vector<int> V = get(); // constructs this const vector every time: { 1, 2, 3 }
  auto i = V.erase(std::find(V.begin(), V.end(), 1)); // state split is creater where i is v.end()
  *i;

As of now, the only way to fine-tune the analyzer through CodeChecker (edit `saargs.txt`, add the flag `--saargs saargs.txt` or something like that) is a quite a jarring interface, and is also buggy (like this thing <https://github.com/Ericsson/codechecker/issues/1957>). Eventually, it would be great to make the `-analyzer-checker-option-help` list visible and easily configurable. I wonder however, if I was a newcomer to CodeChecker, what my reaction would be an option that says "Enables exploration of the past-the-end branch for the return value of the erase() method of containers". Like, why wouldn't I enable that? Lets say we add a disclaimer that it could emit false positives. Lets say I stumble across one. Am I supposed to create a bug report, or is this intended behavior? Not the mention that CodeChecker might not be the only tool to have plans to support `-analyzer-checker-option-help`.

To be honest, I probably shouldn't even see these options straight away. Since this option creates state splits we might not be entitled to, we need more testing and data to get to a point where we could safely put in the default interface -- these are literally the traits of what we call alpha checkers/options. Its just simply not finished, and no amount of documentation will change that. Once I have a better feel for how the analyzer behaves or I'm really stuck, I may wanna go and tick a few alpha stuff as a last resort. This is why we should categorize such option under a different flag.

But, as you said in D77150#inline-707863 <https://reviews.llvm.org/D77150#inline-707863>, there is value in these forsaken options after all. They have on occasion found real bugs that saved a lot of time, and we have gotten a lot of feedback on alpha checkers that also helped us fix a lot of issues. But those are still users that decided to opt-in to in-development features. In retrospect, D32905 <https://reviews.llvm.org/D32905> should've been alpha for this very reason as well.

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

https://reviews.llvm.org/D77150