RFR(S) 8204267 - Generate comments in -XX:+PrintInterpreter to link to source code

[coleen.phillimore at oracle.com]

http://cr.openjdk.java.net/~iklam/jdk12/8204267-print-interpreter-comments.v001/src/hotspot/share/compiler/disassembler.cpp.udiff.html

+ Link *link = new (ResourceObj::C_HEAP, mtInternal) Link(file, line);


Can you use mtCode here?  Why not make Link inherit from 
CHeapObj<mtCode> then just say new?

+ if (_cached_src_lines != NULL) {
+ for (int i=0; i<_cached_src_lines->length(); i++) {
+ os::free((void*)_cached_src_lines->at(i));
+ }
+ _cached_src_lines->clear();
+ } else {
+ _cached_src_lines = new (ResourceObj::C_HEAP, 
mtInternal)GrowableArray<const char*>(0, true);
+ }


Growable Array has a delete operator that will clear_and_deallocate all 
the elements and the array itself (which I don't think this does).   But 
it looks like you're reusing _cached_src_lines for later iterations.  
It's an odd usage.  But you can't resource allocate this?

302 if (_cached_src == NULL || strcmp(_cached_src, file) != 0) {


The indentation is wrong, and this would be a nice place for a comment 
at line 301.

http://cr.openjdk.java.net/~iklam/jdk12/8204267-print-interpreter-comments.v001/src/hotspot/share/compiler/disassembler.hpp.udiff.html

I wonder if checking PrintInterpreter for each macro assembler line will 
slow down generating the interpreter for the normal case. Maybe there is 
an EnableIf<PrintInterpreter> sort of template trick to make it evaporate?

PrintInterpreter is an option in product mode.

http://cr.openjdk.java.net/~iklam/jdk12/8204267-print-interpreter-comments.v001/sample.txt

   0x00007f72c498d527: callq  0x00007f72c498d531             ;;  126:   __ call_VM(noreg,


Should this say what it's calling?

   0x00007f72c498d572: callq  0x00007f72e3df9ea0 = InterpreterRuntime::slow_signature_handler(JavaThread*, Method*, long*, long*)


But this says what it's calling but I don't think this is part of your 
change (or is it?)

That's all the comments I have. I wish you did this many years ago also!
Thanks,
Coleen

On 7/6/18 5:33 PM, Ioi Lam wrote:
> Hi,
>
>
> Since there's interest for this patch, I've finished it for the x86 port:
>
> https://bugs.openjdk.java.net/browse/JDK-8204267
> http://cr.openjdk.java.net/~iklam/jdk12/8204267-print-interpreter-comments.v001/ 
>
>
> You can see a sample at
>
> http://cr.openjdk.java.net/~iklam/jdk12/8204267-print-interpreter-comments.v001/sample.txt 
>
>
> Incorporating this to other CPU ports should be pretty straight 
> forward. You just
> need to do something like:
>
> -#define __ _masm->
> +#define __ Disassembler::hook<MacroAssembler>(__FILE__, __LINE__, 
> _masm)->
>
> I'll leave it to other CPU port maintainers.
>
> I am testing with hs_tier{1,2}.
>
> Thanks
>
> - Ioi
>
>
> On 6/4/18 12:12 AM, Ioi Lam wrote:
>> Hi Folks,
>>
>> I've found it very hard to understand the assembler code printed by 
>> -XX:+PrintInterpreter. Since the C++ source code that generates the 
>> interpreter is fairly well documented, it might be a good idea to 
>> print out the source code along with the assembler code.
>>
>> I've done a quick proof-of-concept by hacking the "__" macro that's 
>> used throughout the CPU-dependent sources.
>>
>> Please let me know if you think this is worth doing. If so, I will 
>> try to finish it up and post a real RFR.
>>
>> Thanks
>> - Ioi
>>
>>
>> https://bugs.openjdk.java.net/browse/JDK-8204267
>> http://cr.openjdk.java.net/~iklam/misc/8204267-print-interpreter-comments.v00 
>>
>> http://cr.openjdk.java.net/~iklam/misc/8204267-print-interpreter-comments.v00/hs_interp.S 
>>
>>
>> Here are some examples (if the mailer screws up the long lines, 
>> please click the above link):
>>
>> ifeq  153 ifeq  [0x00007f830cc93da0, 0x00007f830cc941c0] 1056 bytes
>>
>> mov    (%rsp),%eax
>> add    $0x8,%rsp
>> test   %eax,%eax           ;; 2353:   __ testl(rax, rax);
>> jne    0x00007f830cc94177  ;; 2354:   __ jcc(j_not(cc), not_taken);
>> mov    -0x18(%rbp),%rcx    ;; 2120:   __ get_method(rcx); // rcx 
>> holds method
>> mov    -0x28(%rbp),%rax    ;; 2121:   __ profile_taken_branch(rax, 
>> rbx); // rax holds updated MDP, rbx
>> test   %rax,%rax
>> je     0x00007f830cc93dd8
>> mov    0x8(%rax),%rbx
>> add    $0x1,%rbx
>> sbb    $0x0,%rbx
>> mov    %rbx,0x8(%rax)
>> add    0x10(%rax),%rax
>> mov    %rax,-0x28(%rbp)
>> movswl 0x1(%r13),%edx      ;; 2133:     __ load_signed_short(rdx, 
>> at_bcp(1));
>> bswap  %edx                ;; 2135:   __ bswapl(rdx);
>> sar    $0x10,%edx          ;; 2138:     __ sarl(rdx, 16);
>> movslq %edx,%rdx           ;; 2140:   LP64_ONLY(__ movl2ptr(rdx, rdx));
>> add    %rdx,%r13           ;; 2164:   __ addptr(rbcp, rdx);
>> test   %edx,%edx           ;; 2179:     __ testl(rdx, 
>> rdx);             // check if forward or backward branch
>> jns    0x00007f830cc93eec  ;; 2180:     __ jcc(Assembler::positive, 
>> dispatch); // count only if backward branch
>> mov    0x18(%rcx),%rax     ;; 2184:     __ movptr(rax, Address(rcx, 
>> Method::method_counters_offset()));
>> test   %rax,%rax           ;; 2185:     __ testptr(rax, rax);
>> jne    0x00007f830cc93ead  ;; 2186:     __ jcc(Assembler::notZero, 
>> has_counters);
>> push   %rdx                ;; 2187:     __ push(rdx);
>> push   %rcx                ;; 2188:     __ push(rcx);
>> callq  0x00007f830cc93e09  ;; 2189:     __ call_VM(noreg, 
>> CAST_FROM_FN_PTR(address, InterpreterRuntime::build_method_counters),
>>
>