[PING] Re: RFR(L): 8213084: Rework and enhance Print[Opto]Assembly output
Schmidt, Lutz
lutz.schmidt at sap.com
Fri May 10 15:44:12 UTC 2019
Thank you, Vladimir!
Please find my comments inline and let me know what you think.
A new webrev with all the updates is here: https://cr.openjdk.java.net/~lucy/webrevs/8213084.02/
Please note: the webrev is not based on the most current jdk/jdk! I do not like the idea to "hg pull -u" to a repo state which is known to be broken. Once jdk/jdk is repaired, I will update the webrev in-place (provided there were no serious clashes) and sent a short note.
Regards,
Lutz
On 09.05.19, 21:30, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
Hi Lutz,
Thank you for doing this great work.
I have just small comments:
x86_64.ad - empty change.
File contains whitespace changes for formatting. Not visible in webrev.
nmethod.cpp - LUCY?
+ st->print_cr("LUCY: NULL-oop");
+ tty->print("LUCY NULL-oop");
Oops. Leftover debugging output. Removed. Reads "NULL-oop" now.
nmethod.cpp - use PTR64_FORMAT instead of '0x%016lx'.
Changed.
vmreg.cpp - Use INTPTR_FORMAT instead of %ld for value().
Changed.
disassembler.* - LUCY_OBSOLETE?
+#if defined(LUCY_OBSOLETE) // Used in SAPJVM only
This is fancy code to step backwards in CISC instructions. Used to print a +/- range around a given instruction address. Works reasonably well on s390, will probably not work at all for x86. I could not finally decide to kick it out. But now I did. It's gone.
compilerDefinitions.hpp - I don't see where tier_digit() is used.
I'm surprised myself. Introduced it and then made it obsolete. It's gone.
disassembler.cpp - PrintAssemblyOptions. Why you need to have 'hsdis-' in all options values? You
need to check for invalid value and print help output in such case - it will be very useful if you
forgot a value spelling. Also add line for 'help' value.
The hsdis- prefix existed before I started my work. I just kept it to not hurt anybody's feelings__. Actually, the prefix has a minor practical use. It guards the many "if (strstr(..." instructions from being executed if there is no use. I'm personally not emotionally attached to the hsdis- prefix. I can remove it if you (and the other reviewers) like. Not changed as of now. Awaiting your input.
Printing help text: There is an option (hsdis-help) to request help text printout.
Options parsing doesn't exist here. It's just string comparisons. If one of the predefined strings is found - fine. If not - so what. If you would like to detect unrecognized input, process_options() needs significantly more intelligence. I can do that, but would like to do it in a separate effort. Your opinion?
Do you need next commented lines:
disassembler.cpp -
+// ptrdiff_t _offset;
Deleted.
+// Output suppressed because it messes up disassembly.
+// output()->print_cr("[Disassembling for mach='%s']", (const char*)arg);
Uncommented, would like to keep it. Made the if condition permanently false.
disassembler_s390.cpp -
+// st->fill_to(((st->position()+3*tsize-1)/tsize)*tsize);
Deleted.
compile.cpp -
+// st->print("# "); _tf->dump_on(st); st->cr();
Uncommented.
abstractDisassembler.cpp -
// st->print("0x%016lx", *((julong*)here));
st->print("0x%016lx", *((uintptr_t*)here));
// st->print("0x%08x%08x", *((juint*)here), *((juint*)(here+4)));
Commented lines are gone.
abstractDisassembler.cpp - may be explicit cast (byte*)?:
st->print("%2.2x", *byte);
st->print("%2.2x", *pos);
st->print("0x%02x", *here);
Didn't see the need because the pointers are char* (= address) anyway. And, according to cppreference.com, std::byte is a C++17 feature. We are not there yet.
PTR64_FORMAT ?:
st->print("0x%016lx", *((uintptr_t*)here));
I'm kind of hesitant on that. Nice output alignment clearly depends on this to output exactly 18 characters. Changed other occurrences, so I changed this one as well.
Thanks,
Vladimir
On 5/8/19 8:31 AM, Schmidt, Lutz wrote:
> Dear Community,
>
> may I please request comments and reviews for this change? Thank you!
>
> I have created a new webrev which is based on the current jdk/jdk repo. There was some merge effort. The code which constitutes this patch was not changed. Here's the webrev link:
> https://cr.openjdk.java.net/~lucy/webrevs/8213084.01/
>
> Regards,
> Lutz
>
> On 11.04.19, 23:24, "Schmidt, Lutz" <lutz.schmidt at sap.com> wrote:
>
> Dear All,
>
> this topic was discussed back in Nov/Dec 2018:
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-November/031552.html
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-December/031641.html
>
> Purpose of the discussion was to find out if my ideas are at all regarded useful and desirable.
> The result was mixed, some pro, some con. I let the input from back then influence my work of the last months. In particular, output verbosity can be controlled in a wide range now. In addition to the general -XX:+Print* switches, the amount of output can be adjusted by newly introduced -XX:PrintAssemblyOptions. Here is the list (with default settings):
>
> PrintAssemblyOptions help:
> hsdis-print-raw test plugin by requesting raw output (deprecated)
> hsdis-print-raw-xml test plugin by requesting raw xml (deprecated)
> hsdis-print-pc turn off PC printing (on by default) (deprecated)
> hsdis-print-bytes turn on instruction byte output (deprecated)
>
> hsdis-show-pc toggle printing current pc, currently ON
> hsdis-show-offset toggle printing current offset, currently OFF
> hsdis-show-bytes toggle printing instruction bytes, currently OFF
> hsdis-show-data-hex toggle formatting data as hex, currently ON
> hsdis-show-data-int toggle formatting data as int, currently OFF
> hsdis-show-data-float toggle formatting data as float, currently OFF
> hsdis-show-structs toggle compiler data structures, currently OFF
> hsdis-show-comment toggle instruction comments, currently OFF
> hsdis-show-block-comment toggle block comments, currently OFF
> hsdis-align-instr toggle instruction alignment, currently OFF
>
> Finally, I have pushed my changes to a state where I can dare to request your comments and reviews. I would like to suggest and request that we first focus on the effects (i.e. the generated output) of the changes. Once we got that adjusted and accepted, we can check the actual implementation and add improvements there. Sounds like a plan? Here is what you get:
>
> The machine code generated by the JVM can be printed in three different formats:
> - Hexadecimal.
> This is basically a hex dump of the memory range containing the code.
> This format is always available (PRODUCT and not-PRODUCT builds), regardless
> of the availability of a disassembler library. It applies to all sorts of
> code, be it blobs, stubs, compiled nmethods, ...
> This format seems useless at first glance, but it is not. In an upcoming,
> separate enhancement, the JVM will be made capable of reading files
> containing such code blocks and disassembling them post mortem. The most
> prominent example is an hs_err* file.
> - Disassembled.
> This is an assembly listing of the instructions as found in the memory range
> occupied by the blob, stub, compiled nmethod ... As a prerequisite, a suitable
> disassembler library (hsdis-<platform>.so) must be available at runtime.
> Most often, that will only be the case in test environments. If no disassembler
> library is available, hexadecimal output is used as fallback.
> - OptoAssembly.
> This is a meta code listing created only by the C2 compiler. As it is somewhat
> closer to the Java code, it may be helpful in linking assembly code to Java code.
>
> All three formats can be merged with additional information, most prominently compiler-internal "knowledge" about blocks, related bytecodes, statistics counters, and much more.
>
> Following the code itself, compiler-internal data structures, like oop maps, relocations, scopes, dependencies, exception handlers, are printed to aid in debugging.
>
> The full set of information is available in non-PRODUCT builds. PRODUCT builds do not support OptoAssembly output. Data structures are unavailable as well.
>
> So how does the output actually look like? Here are a few small snippets (linuxx86_64) to give you an idea. The complete output of an entire C2-compiled method, in multiple verbosity variants, is available here:
> http://cr.openjdk.java.net/~lucy/webrevs/8213084/
>
> OptoAssembly output for reference (always on with PrintAssembly):
> =================================================================
>
> 036 B2: # out( B7 B3 ) <- in( B1 ) Freq: 1
> 036 movl RBP, [RSI + #12 (8-bit)] # compressed ptr ! Field: java/lang/String.value (constant)
> 039 movl R11, [RBP + #12 (8-bit)] # range
> 03d NullCheck RBP
>
> 03d B3: # out( B6 B4 ) <- in( B2 ) Freq: 0.999999
> 03d cmpl RDX, R11 # unsigned
> 040 jnb,us B6 P=0.000000 C=5375.000000
>
> PrintAssembly with no disassembler library available:
> =====================================================
>
> [Code]
> [Entry Point]
> 0x00007fc74d1d7b20: 448b 5608 49c1 e203 493b c20f 856f 69e7 ff90 9090 9090 9090 9090 9090 9090 9090
> [Verified Entry Point]
> 0x00007fc74d1d7b40: 8984 2400 a0fe ff55 4883 ec20 440f be5e 1445 85db 7521 8b6e 0c44 8b5d 0c41 3bd3
> 0x00007fc74d1d7b60: 732c 0fb6 4415 1048 83c4 205d 4d8b 9728 0100 0041 8502 c348 8bee 8914 2444 895c
> 0x00007fc74d1d7b80: 2404 be4d ffff ffe8 1483 e7ff 0f0b bee5 ffff ff89 5424 04e8 0483 e7ff 0f0b bef6
> 0x00007fc74d1d7ba0: ffff ff89 5424 04e8 f482 e7ff 0f0b f4f4 f4f4 f4f4 f4f4 f4f4 f4f4 f4f4 f4f4 f4f4
> [Exception Handler]
> 0x00007fc74d1d7bc0: e95b 0df5 ffe8 0000 0000 4883 2c24 05e9 0c7d e7ff
> [End]
>
> PrintAssembly with minimal verbosity:
> =====================================
>
> 0x00007f0434b89bd6: mov 0xc(%rsi),%ebp
> 0x00007f0434b89bd9: mov 0xc(%rbp),%r11d
> 0x00007f0434b89bdd: cmp %r11d,%edx
> 0x00007f0434b89be0: jae 0x00007f0434b89c0e
>
> PrintAssembly (previous plus code offsets from code begin):
> ===========================================================
>
> 0x00007f63c11d7956 (+0x36): mov 0xc(%rsi),%ebp
> 0x00007f63c11d7959 (+0x39): mov 0xc(%rbp),%r11d
> 0x00007f63c11d795d (+0x3d): cmp %r11d,%edx
> 0x00007f63c11d7960 (+0x40): jae 0x00007f63c11d798e
>
> PrintAssembly (previous plus block comments):
> ===========================================================
>
> ;; B2: # out( B7 B3 ) <- in( B1 ) Freq: 1
> 0x00007f48211d76d6 (+0x36): mov 0xc(%rsi),%ebp
> 0x00007f48211d76d9 (+0x39): mov 0xc(%rbp),%r11d
> ;; B3: # out( B6 B4 ) <- in( B2 ) Freq: 0.999999
> 0x00007f48211d76dd (+0x3d): cmp %r11d,%edx
> 0x00007f48211d76e0 (+0x40): jae 0x00007f48211d770e
>
> PrintAssembly (previous plus instruction comments):
> ===========================================================
>
> ;; B2: # out( B7 B3 ) <- in( B1 ) Freq: 1
> 0x00007fc3e11d7a56 (+0x36): mov 0xc(%rsi),%ebp ;*getfield value {reexecute=0 rethrow=0 return_oop=0}
> ; - java.lang.String::charAt at 8 (line 702)
> 0x00007fc3e11d7a59 (+0x39): mov 0xc(%rbp),%r11d ; implicit exception: dispatches to 0x00007fc3e11d7a9e
> ;; B3: # out( B6 B4 ) <- in( B2 ) Freq: 0.999999
> 0x00007fc3e11d7a5d (+0x3d): cmp %r11d,%edx
> 0x00007fc3e11d7a60 (+0x40): jae 0x00007fc3e11d7a8e
>
> For completeness, here are the links to
> Bug: https://bugs.openjdk.java.net/browse/JDK-8213084
> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8213084.00/
>
> But please, as mentioned above, first focus on the output. The nitty details of the implementation I would like to discuss after the output format has received some support.
>
> Thank you so much for your time!
> Lutz
>
>
>
>
More information about the hotspot-compiler-dev
mailing list