[14] RFR(S): 8234583: PrintAssemblyOptions isn't passed to hsdis library
Schmidt, Lutz
lutz.schmidt at sap.com
Thu Nov 28 14:31:49 UTC 2019
HI Martin,
thanks for reviewing. I'll go ahead and push.
Regards,
Lutz
On 28.11.19, 14:10, "Doerr, Martin" <martin.doerr at sap.com> wrote:
Hi Lutz,
looks good to me, too. Thanks for fixing.
Best regards,
Martin
> -----Original Message-----
> From: hotspot-compiler-dev <hotspot-compiler-dev-
> bounces at openjdk.java.net> On Behalf Of Schmidt, Lutz
> Sent: Montag, 25. November 2019 21:48
> To: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>; 'hotspot-compiler-
> dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
> Cc: Jean-Philippe BEMPEL <jean-philippe at bempel.fr>
> Subject: [CAUTION] Re: [14] RFR(S): 8234583: PrintAssemblyOptions isn't
> passed to hsdis library
>
> Thanks for the review, Vladimir!
> Still one to go.
> Regards,
> Lutz
>
> On 25.11.19, 21:26, "Vladimir Ivanov" <vladimir.x.ivanov at oracle.com> wrote:
>
>
> > All your other comments are valid. There is an open bug to address and
> improve the very basic options parsing:
> https://bugs.openjdk.java.net/browse/JDK-8223765 This task was split off
> from JDK-8213084.
> >
> > I would like to cover the improvements you suggest when working on
> that bug. To make _print_raw work correctly, I suggest to just move
> > if (_optionsParsed) return;
> > a bit further down. The help text should be printed only once anyway.
>
> Ok, I'm fine with addressing it later. And thanks for taking care of it.
>
> > Here is a new webrev iteration. It reflects what I suggest:
> https://cr.openjdk.java.net/~lucy/webrevs/8234583.01/
>
> Looks good.
>
> Best regards,
> Vladimir Ivanov
>
> > On 25.11.19, 19:30, "Vladimir Ivanov" <vladimir.x.ivanov at oracle.com>
> wrote:
> >
> > Thanks for the clarifications, Lutz.
> >
> > So, I assume you have a typo in the patch then:
> >
> > + if ((options() == NULL) || (strlen(options()) == 0)) {
> > + // We need to fill the options buffer for each newly created
> > + // decode_env instance. The hsdis_* library looks for options
> > + // in that buffer.
> > + collect_options(Disassembler::pd_cpu_opts());
> > + collect_options(PrintAssemblyOptions);
> > + }
> >
> > It performs collect_options() calls only if _option_buf is either NULL
> > or "\0".
> >
> > Also, what about the following updates of instance members?
> >
> > if (strstr(options(), "print-raw")) {
> > _print_raw = (strstr(options(), "xml") ? 2 : 1);
> > }
> >
> > if (strstr(options(), "help")) {
> > _print_help = true;
> > }
> >
> > BTW should _print_help (along with _helpPrinted) be better turned
> into
> > static member?
> >
> > Can we make _option_buf static as well? Or do we want to keep a
> > defensive copy to pass into hsdis.so?
> >
> > As an alternative approach to fix the bug, we could create a golden
> copy
> > during parsing instead and then just copy it to _option_buf as part of
> > decode_env initialization.
> >
> > Best regards,
> > Vladimir Ivanov
> >
> > On 25.11.2019 19:59, Schmidt, Lutz wrote:
> > > Hi Vladimir,
> > >
> > > I'm happy to elaborate in more detail about the issue and the fix.
> > >
> > > For each decode_env instance which is constructed,
> process_options() is called. It collects the disassembly options from various
> sources (Disassembler::pd_cpu_opts() and PrintAssemblyOptions), storing
> them in the private member "char _option_buf[512]".
> > >
> > > Further processing derives static flag settings from these options.
> Being static, these flags need to be set only once, not every time a
> decode_env is constructed.
> > >
> > > But that's just one part of the story. It was not taken into account
> that _option_buf is passed to and analyzed by hsdis-<platform>.so as well.
> That requires _option_buf to be filled every time a decode_env is
> constructed.
> > >
> > > Moving
> > > if (_optionsParsed) return;
> > > after the collect_options() calls heals this deficiency.
> > >
> > > I added the guards you question as additional "safety net". After
> looking at the code again I must admit the guards are not necessary.
> _option_buf can never be NULL and every invocation of process_options() is
> directly preceded by a memset(_option_buf, 0, sizeof(_option_buf)). I can
> remove the guards if you like.
> > >
> > > Please let me know if there are any more questions to be answered.
> > >
> > > Thanks,
> > > Lutz
> > >
> > >
> > > On 25.11.19, 17:05, "Vladimir Ivanov"
> <vladimir.x.ivanov at oracle.com> wrote:
> > >
> > > Lutz,
> > >
> > > Can you elaborate, please, how the patch fixes the problem?
> > >
> > > Why did you decide to add the following guards?
> > >
> > > + if ((options() == NULL) || (strlen(options()) == 0)) {
> > >
> > > Best regards,
> > > Vladimir Ivanov
> > >
> > > On 25.11.2019 17:06, Schmidt, Lutz wrote:
> > > > Dear all,
> > > >
> > > > may I please request reviews for this small change, fixing a
> regression in the disassembler. Parameters to the hsdis-<platform> library
> were not passed on.
> > > >
> > > > The change was verified to fix the issue by the reporter (Jean-
> Philippe Bempel, on CC:). jdk/submit tests pending...
> > > >
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8234583
> > > > Webrev:
> https://cr.openjdk.java.net/~lucy/webrevs/8234583.00/
> > > >
> > > > Thank you,
> > > > Lutz
> > > >
> > >
> > >
> >
> >
>
More information about the hotspot-compiler-dev
mailing list