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:
> > 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.
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
