Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8022441

Bad code generated for certain interpreted CRC intrinsics, 2 cases

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: P3 P3
    • hs25
    • hs25
    • hotspot
    • b47
    • x86
    • generic

        This was detected in Graal, as yet not reproduced in the plain compiler despite some efforts, but it is definitely a bug:

        (1) [corrected email from original reporter]
        The symptom is a crash when Graal compiles a call to 'java.util.zip.CRC32.update(int b)'. By stepping through the call in gdb, I see that execution ends up in the code produced by InterpreterGenerator::generate_CRC32_update_entry(). Everything seems reasonable until line 883[1]. I believe that line should be:

           __ mov(rsp, r13); // set sp to sender sp

        (2) [second email reporting second issue]
        There's also an issue with InterpreterGenerator::generate_CRC32_updateBytes_entry() in templateInterpreter_x86_64.cpp. I've fixed it in the downstream Graal repo but it should probably be pushed upstream repo as well. The problem is that the addptr assembler instruction was being used to add a 32-bit value from the stack to a 64-bit value in a register. As far as I can tell, there is no format of AMD64 addq instruction that supports this.

        The supplied patch for this bug, tested in Graal, is:

        diff -r 78da293f6efa -r 5c153c59ba62 src/cpu/x86/vm/templateInterpreter_x86_64.cpp
        --- a/src/cpu/x86/vm/templateInterpreter_x86_64.cpp Tue Aug 06 18:32:04 2013 +0200
        +++ b/src/cpu/x86/vm/templateInterpreter_x86_64.cpp Tue Aug 06 21:40:09 2013 +0200
        @@ -995,20 +995,24 @@
             const Register crc = c_rarg0; // crc
             const Register buf = c_rarg1; // source java byte array address
             const Register len = c_rarg2; // length
        + const Register off = len; // offset (never overlaps with 'len')
         
             // Arguments are reversed on java expression stack
        - __ movl(len, Address(rsp, wordSize)); // Length
             // Calculate address of start element
             if (kind == Interpreter::java_util_zip_CRC32_updateByteBuffer) {
               __ movptr(buf, Address(rsp, 3*wordSize)); // long buf
        - __ addptr(buf, Address(rsp, 2*wordSize)); // + offset
        + __ movl(len, Address(rsp, 2*wordSize)); // offset
        + __ addq(buf, len); // + offset
               __ movl(crc, Address(rsp, 5*wordSize)); // Initial CRC
             } else {
               __ movptr(buf, Address(rsp, 3*wordSize)); // byte[] array
               __ addptr(buf, arrayOopDesc::base_offset_in_bytes(T_BYTE)); // + header size
        - __ addptr(buf, Address(rsp, 2*wordSize)); // + offset
        + __ movl(len, Address(rsp, 2*wordSize)); // offset
        + __ addq(buf, len); // + offset
               __ movl(crc, Address(rsp, 4*wordSize)); // Initial CRC
             }
        + // Can now load 'len' since we're finished with 'off'
        + __ movl(len, Address(rsp, wordSize)); // Length
         
             __ super_call_VM_leaf(CAST_FROM_FN_PTR(address, StubRoutines::updateBytesCRC32()), crc, buf, len);
             // result in rax


              drchase David Chase (Inactive)
              drchase David Chase (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Created:
                Updated:
                Resolved: