[14] RFR(S): 8234583: PrintAssemblyOptions isn't passed to hsdis library

[Schmidt, Lutz]

Thank you, Jean-Philippe, for checking once again.
Regards,
Lutz

From: Jean-Philippe BEMPEL <jean-philippe at bempel.fr>
Date: Tuesday, 26. November 2019 at 10:19
To: Lutz Schmidt <lutz.schmidt at sap.com>
Cc: Vladimir Ivanov <vladimir.x.ivanov at oracle.com>, "hotspot-compiler-dev at openjdk.java.net" <hotspot-compiler-dev at openjdk.java.net>
Subject: Re: [14] RFR(S): 8234583: PrintAssemblyOptions isn't passed to hsdis library

Hello,

With last review, still works for me.

Thanks

On Mon, Nov 25, 2019 at 9:47 PM Schmidt, Lutz <lutz.schmidt at sap.com<mailto:lutz.schmidt at sap.com>> wrote:
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<mailto: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<mailto: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<mailto: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
    >      >      >
    >      >
    >      >
    >
    >