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

[Ioi Lam]

Oops, sorry I missed the changes in Disassembler::decode(). The changes 
look good.

Thanks
- Ioi

On 12/16/15 5:29 AM, Vladimir Ivanov wrote:
> 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()?
>>>
>>>
>>