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

[Coleen Phillimore]

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.

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.

  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> 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
>> > Webrev: http://cr.openjdk.java.net/~mgerdin/8027252/webrev.0/
>> > Testing:
>> > vm.quick.testlist -Xint on linux-i586 and x86_64(running)
>> > Thanks
>> > /Mikael
>>
>
>