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

[Vitaly Davidovich]

Right, that's the one.  If it's unsupported, agree, not worth it.

Thanks

Sent from my phone
On Oct 28, 2013 9:47 AM, "Mikael Gerdin" <mikael.gerdin at oracle.com> wrote:

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