RFR: 8274986: max code printed in hs-err logs should be configurable

[Doug Simon]

On Mon, 11 Oct 2021 03:37:28 GMT, Tom Rodriguez <never at openjdk.org> wrote:

>> This PR adds a `ErrorLogPrintCodeLimit` (develop) option for configuring the amount of code printed in a hs-err log file. There's a hard limit of 10 so that the buffer used to avoid duplicates in `VMError::print_code` is stack allocated.
>> In addition, the Java stack is also scanned when considering code to print as the native stack may not have any Java compiled frames. For example, a transition into the VM through a RuntimeStub can prevent the native stack walk from seeing the frames above the stub.
>> The MachCodeFramesInErrorFile test has been made more robust in terms of validating its expectations of how C2 intrinsifies methods.
>> 
>> There's one other minor change to address [this comment](https://github.com/openjdk/jdk/pull/5446#issuecomment-938518814).
>
> src/hotspot/share/code/codeBlob.cpp line 658:
> 
>> 656:     if (st == tty) {
>> 657:       nm->print_nmethod(verbose);
>> 658:     }
> 
> I think this should be something like this:
> 
>      ResourceMark rm;
>      st->print(INTPTR_FORMAT " is at entry_point+%d in (nmethod*)" INTPTR_FORMAT,
>                p2i(addr), (int)(addr - nm->entry_point()), p2i(nm));
> -    if (verbose) {
> -      st->print(" for ");
> -      nm->method()->print_value_on(st);
> -    }
>      st->cr();
> -    nm->print_nmethod(verbose);
> +    if (verbose && st == tty) {
> +      nm->print_nmethod(verbose);
> +    } else {
> +      nm->print_on(st);
> +    }
>      return;
>    }
>    st->print_cr(INTPTR_FORMAT " is at code_begin+%d in ", p2i(addr), (int)(addr - code_begin()));

I don't think so. As far as I can see, the intent is to have a single line of output for an nmethod followed by an optional " for ..." suffix when `verbose == true`.
The `nm->print_nmethod(verbose)` call then prints extra multi-line detail for the nmethod with the number of lines printed governed by `verbose`.
This code seems like it went from being hard coded to always go to `tty` and was then parameterized to go to an arbitrary stream but the evolution accidentally overlooked some code that still goes to `tty`.
I don't want to make extensive changes here as there really should be a single effort to rationalize all dumping to ensure it's parameterized.

> src/hotspot/share/runtime/globals.hpp line 1339:
> 
>> 1337:           "number of stack frames to print in VM-level stack dump")         \
>> 1338:                                                                             \
>> 1339:   develop(int, ErrorLogPrintCodeLimit, 3,                                   \
> 
> Shouldn't this be at least diagnostic?  develop flags are fairly useless.

I chose develop to be consistent with `StackPrintLimit`.
What's the process for adding a diagnostic flag @dholmes-ora ? Should we change both `StackPrintLimit` and `ErrorLogPrintCodeLimit` to be diagnostic? I agree with Tom that having these as develop flags is of limited use.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5875