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

[Vladimir Ivanov]

Thanks, Ioi and Vladimir.

Best regards,
Vladimir Ivanov

On 12/16/15 9:39 PM, Ioi Lam wrote:
> 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()?
>>>>
>>>>
>>>
>