Ping.
On Wed, Jul 30, 2014 at 1:08 PM, Manuel Klimek <[email protected]> wrote: > Jordan, do you think we need to make the marking non-leaking before > checking this in? > > I think the right solution to fix the leakage of markers is to fix the > handling of lifetime extended temporaries, which unfortunately needs a fix > to handling lifetime of temporaries (and inlining temp dtors) first, which > is another patch that will probably take a while to work through and get in. > > If you have a different proposal on how to fix the marking only, I'm all > ears; so far I haven't been able to think of an easy one - we could try to > encode the relationship between the materializetemporary and the > cxxbindtemporary, but that is super-complex (conditional operators, etc), > and I think I won't get it right before having all tests passing for the > handling of lifetime extended temporaries in the cfg. > > Cheers, > /Manuel > > > > On Tue, Jul 29, 2014 at 11:41 AM, Manuel Klimek <[email protected]> wrote: > >> On Mon, Jul 28, 2014 at 9:24 PM, Manuel Klimek <[email protected]> wrote: >> >>> On Mon, Jul 28, 2014 at 8:09 PM, Jordan Rose <[email protected]> >>> wrote: >>> >>>> >>>> On Jul 28, 2014, at 9:36 , Manuel Klimek <[email protected]> wrote: >>>> >>>> On Mon, Jul 28, 2014 at 6:27 PM, Jordan Rose <[email protected]> >>>> wrote: >>>> >>>>> >>>>> On Jul 28, 2014, at 7:56 , Manuel Klimek <[email protected]> wrote: >>>>> >>>>> On Mon, Jul 28, 2014 at 4:01 PM, Manuel Klimek <[email protected]> >>>>> wrote: >>>>> >>>>>> On Mon, Jul 28, 2014 at 3:43 PM, Manuel Klimek <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> Some more context: >>>>>>> It seems like we already need to do that when building the CFG: for >>>>>>> the lifetime extended object we must not emit the destructor at the end >>>>>>> of >>>>>>> the full expression, which we currently do. >>>>>>> >>>>>> >>>>>> And it looks like we're already trying to do that by handing >>>>>> BindToTemporary down. I'm investigating some more... >>>>>> >>>>> >>>>> Found bug, separate patch sent (although right now I think it's also >>>>> still wrong). >>>>> I think we need to fix how we handle lifetime extended temporaries >>>>> before we can really do anything here - the main bug in lifetime extended >>>>> temporary handling is in connecting the MaterializeTemporaryExpr to the >>>>> right CXXBindTemporaryExpr (if it exists). Once that problem is solved, we >>>>> can simple mark the CXXBindTemporary that is lifetime extended once we hit >>>>> the MaterializeTempraryExpr in the static analyzer. >>>>> >>>>> >>>>> I'd really like the CFG to be correct, not something that we have to >>>>> reconstruct correctness for in the analyzer. >>>>> >>>> >>>> Yes, but it's currently not correct. >>>> >>>> >>>>> We should be able to statically tell which temporary a >>>>> MaterializeTemporaryExpr is going to bind to, right? Or rather, we should >>>>> be able to tell whether or not a given CXXBindTemporaryExpr will be >>>>> destroyed or bound if it is executed. >>>>> >>>> >>>> Yes, fully agreed, but first we have to fix how to handle lifetime >>>> extended temporaries - the fix *is* getting that information correctly out >>>> of the AST. >>>> I'd rather not mix this patch with a fix for lifetime extended >>>> temporaries, and I'm not sure continuing to maintain this patch until after >>>> I fixed lifetime extended temps is a good idea. >>>> >>>> >>>> >>>> Agreed. I was just objecting to "mark the CXXBindTemporary...once we >>>> hit the MaterializeTemporaryExpr in the static analyzer". It seems like we >>>> would *never* add a destructor for that temporary in the first place, >>>> so we wouldn't have to mark it as skipped. Or did you mean "when building >>>> the CFG"? >>>> >>> >>> Ah, I meant that we need to mark the constructor to be able to not mark >>> the destructor from the constructor. If that makes sense :) Currently we >>> mark the constructor as "executed" when we hit it - we don't want to do >>> that if it's executed and then lifetime extended. >>> The alternative is to mark the temporary constructor as executed, but >>> then delete that mark when we hit the lifetime extension. >>> >> >> After some digging through codgen, I found that codegen uses a similar >> approach: >> Basically, when hitting the MaterializeTemporaryExpr, it will first skip >> through to the correctly typed subexpr, and then do an aggregate expression >> visitation, which will implicitly either hit the CXXBindTemporary (which >> then belongs to that MaterializeTemporaryExpr) or go back to the outer >> visitation (which will then lose the connection to the >> MaterializeTemoprary). >> >> So, multiple possible approaches: >> 1. When hitting a MaterializeTemporaryExpr in the static analyzer, we >> drill through to the CXXBindTemporaryExpr (using the cases from codegen >> that let's us continue visitation) and then reset the state that was set >> when we visited the CXXBindTemporaryExpr. >> 2. When building the CFG, somehow introduce a pointer from the >> CXXBindTemporaryExpr to its MaterializeTemporaryExpr (or just note down its >> storage duration); I have no idea how to do that from a data structure >> point of view though... >> >> Cheers, >> /Manuel >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
