NoQ 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());
+ }
----------------
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 :(
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
