I'm sorry, I don't understand what you mean by non-leaking. Do you mean that if we decide not to run a destructor, we shouldn't leave that trace in the ProgramState? I think it's okay because it will just get postponed to when the destructor is actually run later, right?
Jordan On Jul 30, 2014, at 4:08 , 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
