[PING] Re: RFR(L): 8213084: Rework and enhance Print[Opto]Assembly output
Schmidt, Lutz
lutz.schmidt at sap.com
Thu May 16 19:22:58 UTC 2019
Hi Vladimir,
thanks for the extensive testing. And sorry for me neglecting ZERO. I will add a dummy instr_len() function. I saw another potential issue. There is no static initializer for AbstractDisassembler::_show_bytes. What is the correct macro to test for ZERO? Is it just "#ifdef ZERO"?
I will prepare a new webrev with just these two additions as delta. But it'll be not before Friday morning, my time.
Thanks,
Lutz
On 16.05.19, 20:38, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
linux-x64-zero build is broke:
workspace/open/src/hotspot/share/compiler/abstractDisassembler.cpp:332:42: error: 'instr_len' is not a member of 'Assembler'
int instr_size_in_bytes = Assembler::instr_len(pos);
^~~~~~~~~
Other builds and testing are good.
Thanks,
Vladimir
On 5/16/19 9:47 AM, Vladimir Kozlov wrote:
> Nice.
>
> I submitted our tier1-3 testing.
>
> Thanks,
> Vladimir
>
> On 5/16/19 2:55 AM, Schmidt, Lutz wrote:
>> Hi Vladimir,
>>
>> sorry for the delayed reaction on your comments.
>>
>> - now it reads "static unsigned int instr_len()". This change added cpu/s390/assembler_s390.inline.hpp to the list
>> of modified files.
>> - testing from my side will be via the submit repo (BuildId: 2019-05-15-1543576.lutz.schmidt.source, no failures). In
>> addition, I added the patch to our internal builds so that our inhouse testing will cover it (no issues detected last
>> night).
>> - All the "hsdis-" prefixes in the PrintAssemblyOptions are gone, as are "print-pc" and "print-bytes". The latter
>> two were legacy anyway. I kept them for compatibility. But now, without the prefix, there is no compatibility anymore.
>> - Options parsing improvement will be done in a separate effort. I have created JDK-8223765 for that.
>> - there is a new webrev, based on the current jdk/jdk repo: https://cr.openjdk.java.net/~lucy/webrevs/8213084.03/
>>
>> ~thartmann:
>> The disabled code in disassembler_s390.cpp is something I would like to have. So far, I could not find time to make it
>> work reliably. I would like to keep it in as a reminder and a template to build on.
>>
>> Thanks,
>> Lutz
>>
>> On 10.05.19, 23:16, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
>>
>> Hi Lutz,
>> My comments are inlined below.
>> On 5/10/19 8:44 AM, Schmidt, Lutz wrote:
>> > 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/
>> Found one more I missed last time:
>> assembler_s390.hpp: still singed return (on other platforms it was converted to unsigned):
>> static int instr_len(unsigned char *instr);
>> > 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.
>> NP. Please, provide final webrev when you can so that I can run these changes through our testing to
>> make sure no issues are present (especially in builds).
>> > 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.
>> Okay.
>> >
>> > nmethod.cpp - LUCY?
>> >
>> > + st->print_cr("LUCY: NULL-oop");
>> > + tty->print("LUCY NULL-oop");
>> > Oops. Leftover debugging output. Removed. Reads "NULL-oop" now.
>> Okay.
>> >
>> > 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.
>> Okay.
>> >
>> > 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.
>> It is a pain to type long values and annoying to type the same prefix. I think hsdis- prefix is
>> useless because PrintAssemblyOptions is used only for disassembler and there are no values which
>> don't have hsdis- prefix. This is not performance critical code to have a guard (check prefix).
>> And an other commented new line:
>> + // ost->print_cr("PrintAssemblyOptions='%s'", options());
>> >
>> > 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?
>> Got it. I forgot that PrintAssemblyOptions flag accepts string with *list* of values - you can't use
>> if-else or switch without complicating the code.
>> I noticed that PrintAssemblyOptions is defined as ccstr. Why it is not ccstrlist which should be use
>> here? I don't think next comment is correct for ccstr type:
>> http://hg.openjdk.java.net/jdk/jdk/file/ef73702a906e/src/hotspot/share/compiler/disassembler.cpp#l190
>> It would be nice to fix it but you can do it later if you don't want to add more changes.
>> >
>> > 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.
>> okay
>> >
>> > 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
>> >
>> >
>> > 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