baloghadamsoftware marked an inline comment as done. baloghadamsoftware added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542 + BR.markInteresting(It1); + if (const auto &LCV1 = It1.getAs<nonloc::LazyCompoundVal>()) { + BR.markInteresting(LCV1->getRegion()); + } ---------------- NoQ wrote: > baloghadamsoftware wrote: > > NoQ wrote: > > > baloghadamsoftware wrote: > > > > baloghadamsoftware wrote: > > > > > baloghadamsoftware wrote: > > > > > > baloghadamsoftware wrote: > > > > > > > Szelethus wrote: > > > > > > > > NoQ wrote: > > > > > > > > > baloghadamsoftware wrote: > > > > > > > > > > NoQ wrote: > > > > > > > > > > > I'm opposed to this code for the same reason that i'm > > > > > > > > > > > opposed to it in the debug checker. Parent region is an > > > > > > > > > > > undocumented implementation detail of `RegionStore`. It > > > > > > > > > > > is supposed to be immaterial to the user. You should not > > > > > > > > > > > rely on its exact value. > > > > > > > > > > > > > > > > > > > > > > @baloghadamsoftware Can we eliminate all such code from > > > > > > > > > > > iterator checkers and instead identify all iterators by > > > > > > > > > > > regions in which they're stored? Does my improved C++ > > > > > > > > > > > support help with this anyhow whenever it kicks in? > > > > > > > > > > How to find the region where it is stored? I am open to > > > > > > > > > > find better solutions, but it was the only one I could find > > > > > > > > > > so far. If we ignore `LazyCompoundVal` then everything > > > > > > > > > > falls apart, we can remove all the iterator-related > > > > > > > > > > checkers. > > > > > > > > > It's the region from which you loaded it. If you obtained it > > > > > > > > > as `Call.getArgSVal()` then it's the parameter region for the > > > > > > > > > call. If you obtained it as `Call.getReturnValue()` then it's > > > > > > > > > the target region can be obtained by looking at the > > > > > > > > > //construction context// for the call. > > > > > > > > `LazyCompoundVal` and friends seem to be a constantly emerging > > > > > > > > headache for almost everyone. For how long I've spent in the > > > > > > > > analyzer, and have religiously studied conversations and your > > > > > > > > workbook about it, I still feel anxious whenever it comes up. > > > > > > > > > > > > > > > > It would be great to have this documented in the code sometime. > > > > > > > Do you mean `CallEvent::getParameterLocation()` for arguments? > > > > > > > What is the //construction context// for the call? How can it be > > > > > > > obtained? > > > > > > I do not know exactly how many place `LazyCompoundVal` appears, > > > > > > but one place for sure is in > > > > > > `MaterializeTemporaryExpr::getSubExpr()`. What to use there instead? > > > > > I also get it in the `Val` parameter of `checkBind()`. > > > > Now I spent a whole day in vain. You probably mean > > > > `ExprEngine::getObjectUnderConstruction()`, (which takes > > > > `ConstructionContextItem` as its argument) but it turned out that there > > > > are no objects under construction in `checkPostCall()`. (Stack dump > > > > says `constructing_objects` as `null`.) It seems that the //only > > > > working solution// is the current one. I am not opposed to find better > > > > working solutions, but we cannot spend months to completely rewrite > > > > parts of the analyzer for such a simple patch. And note tags are > > > > definitely needed for iterator checkers. > > > "A whole day"? "One simple patch"? Give me a break. > > > > > > We've been discussing this problem since your very first implementation > > > of the iterator checker dozens of patches ago, and i spent //six months// > > > of my full time work trying to make this part of the analyzer operate in > > > an obvious, principled manner, even made a dev meeting talk about it in > > > order to explain how it works. And all you do is keep insisting that your > > > solution is "working" even though literally nobody understands how, even > > > you. > > > > > > Out of all the contributors who bring patches to me every day, only > > > @Szelethus is actively addressing the technical debt. This is not "one > > > simple patch". This has to stop at some point, and i expect you, being a > > > fairly senior contributor at this point, to put at least a slight effort > > > into good engineering practices, apply the necessary amount of critical > > > thinking, take basic responsibility for your code. > > > > > > I don't mind if you address this issue immediately after this patch if > > > you urgently need this patch landed. But i wouldn't like this to go on > > > forever. > > > > > > > dump says `constructing_objects` as `null` > > > > > > You shouldn't be spending the whole day before noticing it. Whenever > > > something isn't working, the first thing you do should be dump the > > > ExplodedGraph and look at it. You would have noticed it right away. > > > > > > Was the object never there or was it removed too early? If there are no > > > objects under construction tracked but you need them tracked, make them > > > tracked for as long as you need. That's the whole point of the > > > objects-under-construction map. > > Sorry, @NoQ, I wrote this comment before starting the WIP patch. I agree > > that we should have clean solutions and I do not like hacking at all. Also > > at the University I do not accept solutions that just work by chance (i.e. > > pointers randomly pointing to memory where it luckily does not crash). > > > > However, unfortunately we are a profit-oriented company where our small > > team has tons of internal customers. Their requests should be top-priority. > > I already got lots of comments from my teammates and bosses that I spend > > too much time on a single topic (iterator checkers). If I cannot increase > > my performance the company may decide that these checkers remain in > > downstream. I am trying very hard to avoid that. The point of open-source > > projects is that many are contributing and get other's contributions for > > free. So, theoretically it should be cheaper than downstream development. > > However, in this project there are very few contributors. So if I have to > > spend 20x the time for open-sourcing every single patch than just > > developing a working solution downstream then open-source development turns > > out to be much more expensive. There is a risk that our company will not > > take that. This is the reason I try to balance between fully proper > > solutions and less proper, but fully tested and working solutions. You must > > understand that I am in a situation where I must do this for not losing the > > possibility to open-source my work. > > > > @Szelethus is in somewhat better situation as a student. The company is not > > so strict with students. The money we spend on students is more of a > > "research" investment but employees are paid for serving the customers. > > Unfortunately, I am not allowed to spend years for something that is not > > directly visible for them. So I am willing to fix this issue properly but I > > need your help because I must be as fast as possible. > > > > Anyway, `constructing_objects` is only `null` because we do not have > > `LocationContext` in the dumping function. > > I wrote this comment before starting the WIP patch > > Dang! I almost knew something was wrong :( Sorry, please accept my apologies > :( > > No problem, I phrased a bit too harsh. Please accept my apologies! I really want to solve this problem now properly, so please follow my comments at that patch. Every day I solve one problem and face two more so the number of problems grows exponentially, just like the virus. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75677/new/ https://reviews.llvm.org/D75677 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits