[PING] Re: RFR(L): 8213084: Rework and enhance Print[Opto]Assembly output
Schmidt, Lutz
lutz.schmidt at sap.com
Tue May 21 10:45:43 UTC 2019
Hi Tobias,
thank you very much for your review! I'll send the patch through jdk/submit one more time, just to be on the safe side (there have been some rebase clashes recently). If successful, I'll push.
Best,
Lutz
On 21.05.19, 12:38, "Tobias Hartmann" <tobias.hartmann at oracle.com> wrote:
Hi Lutz,
this looks good to me.
Best regards,
Tobias
On 20.05.19 10:22, Schmidt, Lutz wrote:
> Hi Vladimir,
>
> I appreciate the time you spent making this change "push-worthy". Thank you for reviewing it!
>
> I am now hoping for a second reviewer to set aside some time and have a look.
>
> Thanks,
> Lutz
>
> On 17.05.19, 16:48, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
>
> Good.
>
> Thanks
> Vladimir
>
> > On May 17, 2019, at 6:19 AM, Schmidt, Lutz <lutz.schmidt at sap.com> wrote:
> >
> > Hi Vladimir,
> > here is what I changed to overcome the ZERO build issues:
> >
> > ----------- 8< ----------------
> > diff -r c0568c492760 src/hotspot/cpu/zero/assembler_zero.hpp
> > --- a/src/hotspot/cpu/zero/assembler_zero.hpp Fri May 17 14:11:44 2019 +0200
> > +++ b/src/hotspot/cpu/zero/assembler_zero.hpp Fri May 17 14:14:06 2019 +0200
> > @@ -37,6 +37,12 @@
> >
> > public:
> > void pd_patch_instruction(address branch, address target, const char* file, int line);
> > +
> > + //---< calculate length of instruction >---
> > + static unsigned int instr_len(unsigned char *instr) { return 1; }
> > +
> > + //---< longest instructions >---
> > + static unsigned int instr_maxlen() { return 1; }
> > };
> >
> > class MacroAssembler : public Assembler {
> > diff -r c0568c492760 src/hotspot/share/compiler/abstractDisassembler.cpp
> > --- a/src/hotspot/share/compiler/abstractDisassembler.cpp Fri May 17 14:11:44 2019 +0200
> > +++ b/src/hotspot/share/compiler/abstractDisassembler.cpp Fri May 17 14:14:06 2019 +0200
> > @@ -61,6 +61,9 @@
> > bool AbstractDisassembler::_show_bytes = false; // set "true" to see what's in memory bit by bit
> > // might prove cumbersome because instr_len is hard to find on x86
> > #endif
> > +#if defined(ZERO)
> > +bool AbstractDisassembler::_show_bytes = false; // set "true" to see what's in memory bit by bit
> > +#endif
> >
> > // Return #bytes printed. Callers may use that for output alignment.
> > // Print instruction address, and offset from blob begin.
> > ----------- >8 ----------------
> >
> > This delta is contained as the only change in the new webrev#04 which is based on the current (13:10 GMT) jdk/jdk repo:
> > https://cr.openjdk.java.net/~lucy/webrevs/8213084.04/
> >
> > Regards,
> > Lutz
> >
> >
> > On 16.05.19, 21:40, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
> >
> > I am not sure about exact parameters but I see build testing uses next:
> >
> > configure --with-jvm-variants=zero --with-jvm-features=-shenandoahgc
> >
> > Vladimir
> >
> >> On 5/16/19 12:22 PM, Schmidt, Lutz wrote:
> >> 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