[Fwd: Request for review (S): CR 6889740 - G1: OpenDS fails with "unhandled exception in compiled code"]

[john cuthbertson - Sun Microsystems]

Hi Tom,

I think you only need the additional variant that takes the void* in 
order to make this a compile time problem. Adding that prototype first 
flagged the other calls in do_oop_store as compile time errors. As long 
as we have that then any call to store_heap_oop added in the future 
would be caught.

JohnC

On 10/28/09 10:35, Tom Rodriguez wrote:
> But don't you need all three variants to turn this into a compile time 
> problem?  I'm ok with what you've done.  You should note that the new 
> variant is only there to deal with the use of NULL since all other oop 
> constants should be using jobject.
>
> tom
>
> On Oct 28, 2009, at 9:35 AM, john cuthbertson - Sun Microsystems wrote:
>
>> Hi Christian,
>>
>> The main reason was just to avoid changing the call-sites and make it 
>> consistent with the other generator routines. I don't have a strong 
>> preference either way.
>>
>> Thanks for looking at the code,
>>
>> JohnC
>>
>> On 10/28/09 01:36, Christian Thalinger wrote:
>>> On Tue, 2009-10-27 at 15:10 -0700, john cuthbertson - Sun Microsystems
>>> wrote:
>>>
>>>
>>>> Can I have a couple of volunteers to review the proposed fix for this
>>>> bug? The webrev can be found at
>>>>
>>>> http://cr.openjdk.java.net/~johnc/6889740/webrev.0/
>>>> .
>>>>
>>>> The issue is that bad code was being generated for the store operation
>>>> in the null case of the aastore bytecode template. The bad code was
>>>> caused by there being only one version of the store_heap_oop routine
>>>> that took a Register as the second argument. When the calling code
>>>> passed in NULL_WORD (0) to this routine the value was used as a 
>>>> Register
>>>> encoding and converted to Register(0), which is rax. Thus the 
>>>> generated
>>>> store was "mov (dst), $rax" instead of "mov (dst), $0x0". This is
>>>> normally not a problem as the preceding code in the template 
>>>> fetches the
>>>> value to be stored into rax. When the G1 pre-barrier code calls the
>>>> runtime, however, the value in rax can be overwritten and the heap can
>>>> become corrupted.
>>>>
>>>>
>>>
>>> Why do you actually pass in a src and then assert on it's value?
>>>
>>> +void MacroAssembler::store_heap_oop(Address dst, intptr_t src) {
>>> +  assert(src == NULL_WORD, "use something else otherwise");
>>>
>>> It seems it must be null anyway and we could use something like:
>>>
>>> void MacroAssembler::store_heap_oop_null(Address dst) {
>>>
>>> -- Christian
>>>
>>>
>>>
>>
>