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

[Tom Rodriguez]

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
>>
>>
>>
>