[9] RFR (S): 8071374: Native disassembler implementation may be not thread-safe

[Vladimir Ivanov]

Ioi,

Sorry for the confusion, the fix does exactly that - adds ttyLocker in 
all 3 Disassembler::decode variants [1].

Other changes are cleanups - use ttyLocker for multi-line dumps.

Also, what do you think about result handler output w/ 
-XX:+PrintSignatureHandlers? Leave it as is?

Best regards,
Vladimir Ivanov

[1] 
http://cr.openjdk.java.net/~vlivanov/8071374/webrev.00/src/share/vm/compiler/disassembler.cpp.udiff.html

On 12/16/15 3:14 AM, Ioi Lam wrote:
> Hi Vladimir,
>
> Thanks for fixing this bug. I have a suggestion: instead of doing this:
>
> 2642     ttyLocker ttyl;
> 2643     tty->print_cr("implicit exception happened at " INTPTR_FORMAT,
> p2i(pc));
> 2644     print();
> 2645     method()->print_codes();
> 2646     print_code();
> 2647     print_pcs();
>
> Maybe the ttyLocker should be acquired inside Disassembler::decode?
>
> Your code has the advantage of keeping the related info in the same part
> of the print-out, but that's a different problem than the one you want
> to solve in this bug. If another thread forgets to take the tty lock
> before calling Disassembler::decode, it would still cause your code to
> crash.
>
> Thanks
> - Ioi
>
>
> On 12/15/15 4:30 AM, Vladimir Ivanov wrote:
>> http://cr.openjdk.java.net/~vlivanov/8071374/webrev.00
>> https://bugs.openjdk.java.net/browse/JDK-8071374
>>
>> Disassembler wraps some native disassembler, which is not necessarily
>> thread-safe. It's not a problem for -XX:+PrintAssembly since access
>> from compilers is serialized by Compile_lock.
>>
>> It is not the case anymore when there are calls from runtime (e.g.,
>> with -XX:+PrintSignatureHandlers). The problem can manifest as a
>> failure to parse instruction stream.
>>
>> The fix is to serialize access to Disassembler on tty_lock.
>>
>> Considering most of the calls to Disassembler::decode are performed
>> under tty_lock (which has the lowest rank), it's too burdensome to
>> introduce a dedicated lock and a new rank to please deadlock detection
>> logic.
>>
>> Also, some cleanups are included.
>>
>> Testing: failing test case from the report, JPRT.
>>
>> Thanks!
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> PS: I noted that the following code usually dumps some garbage at the
>> end of the code block:
>>
>> src/share/vm/interpreter/interpreter.cpp:
>> void SignatureHandlerLibrary::add(const methodHandle& method) {
>> ...
>>   tty->print_cr(" --- associated result handler ---");
>>   address rh_end = rh_begin;
>>   while (*(int*)rh_end != 0) {
>>     rh_end += sizeof(int);
>>   }
>>   Disassembler::decode(rh_begin, rh_end);
>>
>> $ java -XX:+PrintSignatureHandlers ...
>> ...
>> argument handler #0 for: static java.lang.Object.registerNatives()V
>> (fingerprint = 349, 11 bytes generated)
>>   0x0000000106d55e60: movabs $0x106c1b118,%rax
>>   0x0000000106d55e6a: retq
>>  --- associated result handler ---
>>   0x0000000106c1b118: retq  // T_VOID:   _native_abi_to_tosca[6]
>>   0x0000000106c1b119: retq  // T_FLOAT:  _native_abi_to_tosca[7]
>>   0x0000000106c1b11a: retq  // T_DOUBLE: _native_abi_to_tosca[8]
>>   // T_OBJECT/T_ARRAY: _native_abi_to_tosca[9]
>>   0x0000000106c1b11b: mov    0x10(%rbp),%rax
>>   0x0000000106c1b11f: retq
>>
>> === end of AbstractInterpreter::_native_abi_to_tosca[]
>>
>> Garbage until (*(int*)rh_end) == 0:
>>
>>   0x0000000106c1b120: rex add %eax,(%rax)
>>   0x0000000106c1b123: add    %cl,%ah
>>   0x0000000106c1b125: int3
>>   0x0000000106c1b126: int3
>>   0x0000000106c1b127: int3
>>   0x0000000106c1b128: insl   (%dx),%es:(%rdi)
>>   0x0000000106c1b129: sub    $0x10521,%eax
>>   0x0000000106c1b12e: add    %al,(%rax)
>>   0x0000000106c1b130: (bad)
>>   0x0000000106c1b131: (bad)
>>   0x0000000106c1b132: (bad)
>>   0x0000000106c1b133: dec    %esp
>>   0x0000000106c1b135: int3
>>   0x0000000106c1b136: int3
>>   0x0000000106c1b137: int3
>>
>> Maybe add some padding in either CodeletMark::~CodeletMark or
>> TemplateInterpreterGenerator::generate_all()?
>>
>>
>