[8u] RFR: JDK-8220786: Create new switch to redirect error reporting output to stdout or stderr

[Thomas Stüfe]

Hi,

just chiming in from the side. I agree that this is an error and should be
fixed in head. But I honestly think the error is rather benign. The error
would cause an error log file that happens to have the numerical fd=3 to be
not closed. But we are going to abort(3) anyway, which will properly close
that file descriptor. Also, the chance to even get that fd is
astronomically low.

I personally would just go ahead with the downport and let the second fix
trickle through later, but of course that is not up to me.

Cheers, Thomas


On Wed, Jun 2, 2021 at 1:19 AM Hohensee, Paul <hohensee at amazon.com> wrote:

> Also looks to me like the test in vmError.cpp should be "> 2" rather than
> "> 3". The original review thread
>
>
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-March/033293.html
>
> has no discussion of why it's 3 instead of 2, but, since "> 3" is in tip,
> I'd rather keep "> 3" in this backport, fix it in tip, then backport the
> fix to 11u and 8u. It needs to be fixed in tip and 11u anyway, and doing it
> the formal way will keep things organized.
>
> Otherwise lgtm.
>
> Thanks,
> Paul
>
> -----Original Message-----
> From: jdk8u-dev <jdk8u-dev-retn at openjdk.java.net> on behalf of Volker
> Simonis <volker.simonis at gmail.com>
> Date: Tuesday, June 1, 2021 at 12:37 PM
> To: jdk8u-dev <jdk8u-dev at openjdk.java.net>
> Subject: [8u] RFR: JDK-8220786: Create new switch to redirect error
> reporting output to stdout or stderr
>
> Hi,
>
> can I please get a review for this backport?
>
> This change initially went into jdk 13 and has already been downported
> to jdk 11u more than a year ago. But as jdk8u is still used a lot and
> increasingly in containerized environments I think it still makes a
> lot of sense to downport it to jdk 8u as well.
>
> If the JVM crashes in a containerized environment with its ephemeral
> file system it becomes impossible to do any post mortem analysis of
> the problem. In such situations the ability to dump the hs_err file to
> stdout/stderr is very useful, because many container environments log
> the stdout/stderr stream by default.
>
> Original issue: https://bugs.openjdk.java.net/browse/JDK-8220786
> Original CSR: https://bugs.openjdk.java.net/browse/JDK-8220787
> Original change: http://hg.openjdk.java.net/jdk/jdk/rev/cf75ea6af695
>
> jdk11 backport: https://bugs.openjdk.java.net/browse/JDK-8238531
> jdk11 CSR: https://bugs.openjdk.java.net/browse/JDK-8238532
> jdk11 change:
> https://hg.openjdk.java.net/jdk-updates/jdk11u/rev/783a56f3ac1f
>
> jdk8 backport: https://bugs.openjdk.java.net/browse/JDK-8268055
> jdk8 CSR: https://bugs.openjdk.java.net/browse/JDK-8268058
> jdk8 webrev:
> http://cr.openjdk.java.net/~simonis/webrevs/2021/8220786-jdk8u/
>
> I had to do quite some, but trivial changes to the jdk 11 backport
> (which I started with) to fit it into jdk 8u:
>
> arguments.cpp
> -------------------
> In jdk 11 "FLAG_SET_CMDLINE(boolean, ...)" expands to "JVMFlag::Error
> boolAtPut(JVMFlagsWithType flag, bool value, JVMFlag::Flags origin)"
> which returns a "JVMFlag::Error" and can be checked for success. In
> jdk 8,  "FLAG_SET_CMDLINE(boolean, ...)" expands to "void
> boolAtPut(CommandLineFlagWithType flag, bool value, Flag::Flags
> origin)" which returns void, so there's nothing to check for...
>
> vmError.cpp
> ----------------
> I think the initial implementation has a bug in the following code:
>
> -    if (fd_log != -1) {
> +    if (fd_log > 3) {
>        close(fd_log);
>
> I think the intention is to close the log stream if it was not stdout
> (i.e. "1") or stderr (i.e. "2") but it checks for "fd_log > 3".
>
> I've fixed this in the downport but I'm happy to revert the fix if
> somebody has a good explanation for the original code:
>
> -    if (log.fd() != defaultStream::output_fd()) {
> +    if (log.fd() > 2) {
>
> ErrorFileRedirectTest.java
> ---------------------------------
> - path/package shuffling for the test library
> - use "Platform.isDebugBuild()" instead of "@requires (vm.debug ==
> true)" which is not supported in jdk8
> - remove "-XX:-CreateCoredumpOnCrash" option because it doesn't exist in
> jdk 8
> - use "-XX:ErrorHandlerTest=12" instead of "-XX:ErrorHandlerTest=14"
> because "14" is not available in jdk 8. But "12" still generates a
> SIGSEV by reading from memory address 0.
> - check for "T H R E A D" in the output because jdk 8 has no "S U M M
> A R Y" section.
>
> The jdk 8 CSR (https://bugs.openjdk.java.net/browse/JDK-8268058) has
> been reviewed and waits for approval.
>
> Thank you and best regards,
> Volker
>
>