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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
