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
