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

Szelethus accepted this revision. Szelethus added a reviewer: gamesh411. Szelethus added a comment. LGTM on my end, but please wait for approval from either @gamesh411 or @NoQ. In D77150#2191049 , @baloghadamsoftware wrote: > However, I think we should

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

baloghadamsoftware updated this revision to Diff 282642. baloghadamsoftware added a comment. Option made hidden. However, I think we should create a category "advanced" for options which affect the internal modeling. Our users are programmers and the advanced among them should see this option

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

whisperity added a comment. In D77150#2149870 , @dkrupp wrote: > Since the analyzer cannot cannot model the size of the containers just yet > (or precisely enough), what we are saying with this checker is to "always > check the return value of the

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

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. Let me double down -- just because we let some select people know about an option, I still don't think it should be user facing. I'm strongly against the philosophy that a

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

baloghadamsoftware added a comment. Now I made some measurements about the false positives this option adds. For `BitCoin` the increase was `-1`. I do not know how it happened, but it reduced the number of false positives by one. For `LLVM/Clang` the increase was `15` which is less than `4%`

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

baloghadamsoftware updated this revision to Diff 278396. baloghadamsoftware added a comment. Rebased. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77150/new/ https://reviews.llvm.org/D77150 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

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

baloghadamsoftware added a comment. I am trying to measure the amount of extra false positive on real code (not artificial examples) of this option. The modeling of container size may reduce it, of course, but we have no other option to find this bug: even loop-unrolling or loop-widening is

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

dkrupp requested changes to this revision. dkrupp added a comment. This revision now requires changes to proceed. Since the analyzer cannot cannot model the size of the containers just yet (as I believe this is the case now), what we are saying with this checker is to "always check the return

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

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

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

NoQ added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:647-653 CmdLineOption ]>, baloghadamsoftware wrote: > Szelethus wrote: > > Szelethus wrote: > > > NoQ wrote: > > > > baloghadamsoftware wrote: > > > > >

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

baloghadamsoftware updated this revision to Diff 275406. baloghadamsoftware added a comment. Rebased. Tests added. Fixed to work on `std::vector`-like and `std::deque`-like containers as well. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77150/new/ https://reviews.llvm.org/D77150

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

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:647-653 CmdLineOption ]>, Szelethus wrote: > Szelethus wrote: > > NoQ wrote: > > >

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

Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:647-653 CmdLineOption ]>, Szelethus wrote: > NoQ wrote: > > baloghadamsoftware wrote: > > > Szelethus wrote: > > > > Ah, okay, I see which one you refer

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

Szelethus added a comment. Please mind that my inline is NOT an objection to this patch. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:647-653 CmdLineOption ]>, NoQ wrote: > baloghadamsoftware wrote: > > Szelethus wrote: > > > Ah,

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

NoQ added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:620-624 + "AggressiveEraseModeling", + "Enables exploration of the past-the-end branch for the " + "return value of the erase()

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

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:647-653 CmdLineOption ]>, Szelethus wrote: > Ah, okay, I see which one you refer to. We

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

Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:647-653 CmdLineOption ]>, Ah, okay, I see which one you refer to. We should totally make this non-user facing as well. Repository: rG LLVM Github

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

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:692-699 + std::tie(StateEnd, StateNotEnd) = State->assume(*RetEnd); + if (StateEnd) { +

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

Szelethus added a comment. Some tests would be appreciated, It really helps me understand whats going on. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:620-624 + "AggressiveEraseModeling", + "Enables exploration of the

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

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:692-699 + std::tie(StateEnd, StateNotEnd) = State->assume(*RetEnd); + if (StateEnd) { +C.addTransition(StateEnd); + } + if

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

baloghadamsoftware marked an inline comment as done. baloghadamsoftware added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:780 + if (AggressiveEraseModeling) { +if (const auto *CData = getContainerData(State, ContReg)) {

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

martong added a comment. Do you have a lit test as well? It would be useful to have one that is similar to the one you mention in the review's summary. Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:780 + if (AggressiveEraseModeling) { +if (const

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

baloghadamsoftware created this revision. baloghadamsoftware added reviewers: NoQ, Szelethus. baloghadamsoftware added a project: clang. Herald added subscribers: ASDenysPetrov, martong, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun,