[llvm-commits] [llvm] r78127 - in /llvm/trunk: lib/ExecutionEngine/ExecutionEngine.cpp unittests/ExecutionEngine/ExecutionEngineTest.cpp unittests/ExecutionEngine/Makefile

[Jeffrey Yasskin]

On Tue, Aug 4, 2009 at 10:33 PM, Chris Lattner<clattner at apple.com> wrote:
>
> On Aug 4, 2009, at 9:12 PM, Jeffrey Yasskin wrote:
>
>> On Tue, Aug 4, 2009 at 7:30 PM, Chris Lattner<clattner at apple.com>
>> wrote:
>>>
>>> On Aug 4, 2009, at 4:53 PM, Jeffrey Yasskin wrote:
>>>
>>>> Author: jyasskin
>>>> Date: Tue Aug  4 18:53:16 2009
>>>> New Revision: 78127
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=78127&view=rev
>>>> Log:
>>>> Make ExecutionEngine::updateGlobalMapping(GV, NULL) properly remove
>>>> GV's old
>>>> address from the reverse mapping, and add a test that this works
>>>> now.
>>>
>>> Hey Jeffrey,
>>>
>>> Should the GlobalMapping move to using AssertingVH?  I think that
>>> would define this class of bugs away.
>>
>> I'd actually like to move them and the other maps in ExecutionEngine
>> to using CallbackVH so that users don't have to deal with
>> removing/changing global mappings unless the object actually moves.
>
> That would also be nice. :)
>
>> But whether or not I do that, this bug made it impossible to do
>> things like:
>>
>> T1 *obj1 = new T1;
>> engine.addGlobalMapping(GV, obj1);
>> ...
>> engine.updateGlobalMapping(GV, NULL);
>> delete obj1;
>> ...
>> T2 *obj2 = new T2;  // Happens to land at the same address.
>> engine.addGlobalMapping(UnrelatedGV, obj2);
>>
>> and I don't see how AssertingVH would make any of that illegal or
>> work better.
>
> If "updateglobalmapping" to null deletes the entry out of the map,
> then assertingvh should work fine.

Oh, I see what you're going for. Yep, I'll switch those GlobalValue*s
to AssertingVH's (since going to CallbackVHs will take longer) and run
the nightly tests to see what else needs fixing.