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

[Vladimir Ivanov]

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
>      >
>      
>