Review/Comment request (S) 8027252: Crash in interpreter because get_unsigned_2_byte_index_at_bcp reads 4 bytes

[Vitaly Davidovich]

Mikael,

If you're inclined, you can use xchg (on Intel at least) to do a 16 bit
bswap equivalent - would save you the shift instruction and xchg of this
form would be just 1 byte encoding (I.e. instruction size).  Doubt
performance will be noticeable though ...

Sent from my phone
On Oct 25, 2013 2:51 PM, "Mikael Gerdin" <mikael.gerdin at oracle.com> wrote:

> Coleen,
>
> On 25 October 2013 20:33:07 Coleen Phillimore <
> coleen.phillimore at oracle.com> wrote:
>
>>
>> Mikael,
>>
>> This is really good work!  Thank you for narrowing this down from the
>> core file because we were going to have a lot of trouble reproducing this.
>>
>
> Thanks!
>
>
>> The code changes look good but why do you still need the bswapl and shrl?
>>   Doesn't load_unsigned_short load the value without sign extending also?
>>  I'm only guessing what bswapl does from the context though.
>>
>
> Since the bytecode stores big endian values we still need to convert them
> to native (little endian) byte order.
>
> So if we have the bytecode:
> checkcast #6 0xc0 0x00 0x06
>
> After the load we end up with
> 0x00000600
> Bswap turns this to
> 0x00060000
> The shift gets us
> 0x00000006
>
> (if someone could sanity-check this for me it would be great)
>
> /Mikael
>
>
>>   void TemplateTable::locals_index_**wide(Register reg) {
>> -  __ movl(reg, at_bcp(2));
>> +  __ load_unsigned_short(reg, at_bcp(2));
>>     __ bswapl(reg);
>>     __ shrl(reg, 16);
>>     __ negptr(reg);
>>   }
>>
>> Thanks,
>> Coleen
>>
>> On 10/25/2013 12:46 PM, Mikael Gerdin wrote:
>> >
>> >
>> > On 25 October 2013 18:25:47 Christian Thalinger <
>> christian.thalinger at oracle.**com <christian.thalinger at oracle.com>> wrote:
>> >> Good catch.  Is there a similar issue on SPARC?
>> >
>> > I looked through the other platforms, they all seem to do single-byte
>> loads for the indices.
>> > On SPARC I suppose it's because it can't do unaligned multibyte loads.
>> > /Mikael
>> >
>> >>
>> >> On Oct 25, 2013, at 8:55 AM, Mikael Gerdin <mikael.gerdin at oracle.com>
>> wrote:
>> >>
>> >> > Hi all,
>> >> > Initially I'd like to hear people's opinions on this issue. It
>> appears to have surfaced after we've reduced the unnecessary alignment
>> "cushions" for metaspace together with the fact that we are now always able
>> to use the very last bit of a VirtualSpace (in Metaspace).
>> >> > The issue is extremely difficult to reproduce, because it relies on
>> the exact order and size of all metaspace allocations for a run to trigger
>> this problem.
>> >> > Description of the problem:
>> >> > We recently had some mysterious crashes in the GC testing, and I
>> narrowed them down to the following scenario:
>> >> > Imagine that we have a VirtualSpace (for Metaspace)
>> >> > [                     |        bb]
>> >> >                       ConstMethod
>> >> > And that the very last object allocated in such a space is a
>> ConstMethod, and that it has its embedded bytecodes filling up to the very
>> end.
>> >> > Consider the case when the second last bytecode (the one before
>> "Xreturn") is one with an embedded 2-byte index (a constant pool or local
>> variable index) for example:
>> >> > - (bcp) checkcast #6 0xc0 0x00 0x06 - (bcp+3) areturn 0xb0 - (bcp+4)
>> UNMAPPED MEMORY
>> >> > The template interpreter here uses a 4-byte load on intel to read
>> the index embedded in the checkcast:
>> >> > mov    0x1(%esi),%ebx
>> >> > this will read the bytes [bcp+1,bcp+4] and cause a SEGV.
>> >> > I looked through the code for places where we do similar loads of
>> 2-byte indexes and found the same problem for iload_w:
>> >> >   0xe73f7fbb:  mov    0x2(%esi),%ebx
>> >> >   0xe73f7fbe:  bswap  %ebx
>> >> >   0xe73f7fc0:  shr    $0x10,%ebx
>> >> > I looked through most places where we emit
>> >> > __ movl
>> >> > __ bswap
>> >> > and changed the places I could find which were affected.
>> >> > Suggested fix:
>> >> > Replace movl with load_[un]signed_short in the places where we load
>> 2-byte values from the byte code stream.
>> >> > I've verified that load_unsigned_short (which boils down to movzwl,
>> opcodes 0x0f 0xb7 only reads 2 bytes).
>> >> > Buglink: https://bugs.openjdk.java.net/**browse/JDK-8027252<https://bugs.openjdk.java.net/browse/JDK-8027252>
>> >> > Webrev: http://cr.openjdk.java.net/~**mgerdin/8027252/webrev.0/<http://cr.openjdk.java.net/~mgerdin/8027252/webrev.0/>
>> >> > Testing:
>> >> > vm.quick.testlist -Xint on linux-i586 and x86_64(running)
>> >> > Thanks
>> >> > /Mikael
>> >>
>> >
>> >
>>
>>
>
>