Review/Comment request (S) 8027252: Crash in interpreter because get_unsigned_2_byte_index_at_bcp reads 4 bytes
Mikael Gerdin
mikael.gerdin at oracle.com
Fri Oct 25 11:50:39 PDT 2013
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> 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
> >>
> >
> >
>
More information about the hotspot-dev
mailing list