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

[Mikael Gerdin]

Vitaly,

On Friday 25 October 2013 20.44.03 Vitaly Davidovich wrote:
> 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 ...

Did you mean something like:
xchg %bl, %bh

The assembler in HotSpot currently does not implement that particular opcode, 
and since not all X86 registers are low/high byte addressable I'm not sure 
that the extra complexity is worth it.

/Mikael

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