RFR: JDK-8193055 ADD_JVM_ARG_IF_OK always fails

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8193055 ADD_JVM_ARG_IF_OK always fails

Magnus Ihse Bursie
We mistakenly use -XX:-UnlockDiagnosticVMOptions instead of
-XX:+UnlockDiagnosticVMOptions in a ADD_JVM_ARG_IF_OK call. This means
that the test will always fail and the arguments will never be added to
the command line.

Since this has been the case all time along, it's probably not that
important, but it was added to keep down the logging when using a debug
build as boot jdk, which has probably not been tested that much either.

We should either fix this or remove the arguments completely.

Bug: https://bugs.openjdk.java.net/browse/JDK-8193055
Patch inline:
diff --git a/make/autoconf/boot-jdk.m4 b/make/autoconf/boot-jdk.m4
--- a/make/autoconf/boot-jdk.m4
+++ b/make/autoconf/boot-jdk.m4
@@ -354,7 +354,7 @@
    AC_MSG_CHECKING([flags for boot jdk java command] )

    # Disable special log output when a debug build is used as Boot JDK...
-  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions -XX:-UnlockDiagnosticVMOptions
-XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
+  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions -XX:+UnlockDiagnosticVMOptions
-XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])

    # Force en-US environment
    ADD_JVM_ARG_IF_OK([-Duser.language=en
-Duser.country=US],boot_jdk_jvmargs,[$JAVA])

/Magnus
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8193055 ADD_JVM_ARG_IF_OK always fails

David Holmes
Hi Magnus,

On 5/12/2017 8:49 PM, Magnus Ihse Bursie wrote:
> We mistakenly use -XX:-UnlockDiagnosticVMOptions instead of
> -XX:+UnlockDiagnosticVMOptions in a ADD_JVM_ARG_IF_OK call. This means
> that the test will always fail and the arguments will never be added to
> the command line.
>
> Since this has been the case all time along, it's probably not that
> important, but it was added to keep down the logging when using a debug
> build as boot jdk, which has probably not been tested that much either.

LogVMOutput will only be turned on in a debug build if you also enable
specific (mostly debug only) log/print/trace options.

> We should either fix this or remove the arguments completely.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8193055
> Patch inline:
> diff --git a/make/autoconf/boot-jdk.m4 b/make/autoconf/boot-jdk.m4
> --- a/make/autoconf/boot-jdk.m4
> +++ b/make/autoconf/boot-jdk.m4
> @@ -354,7 +354,7 @@
>     AC_MSG_CHECKING([flags for boot jdk java command] )
>
>     # Disable special log output when a debug build is used as Boot JDK...
> -  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions -XX:-UnlockDiagnosticVMOptions
> -XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
> +  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions -XX:+UnlockDiagnosticVMOptions
> -XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])

Fix is fine. But you could probably remove them too. Do you recall why
this was added? It may relate to something now migrated to Unified Logging.

David
-----

>     # Force en-US environment
>     ADD_JVM_ARG_IF_OK([-Duser.language=en
> -Duser.country=US],boot_jdk_jvmargs,[$JAVA])
>
> /Magnus
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8193055 ADD_JVM_ARG_IF_OK always fails

Magnus Ihse Bursie
On 2017-12-05 13:03, David Holmes wrote:

> Hi Magnus,
>
> On 5/12/2017 8:49 PM, Magnus Ihse Bursie wrote:
>> We mistakenly use -XX:-UnlockDiagnosticVMOptions instead of
>> -XX:+UnlockDiagnosticVMOptions in a ADD_JVM_ARG_IF_OK call. This
>> means that the test will always fail and the arguments will never be
>> added to the command line.
>>
>> Since this has been the case all time along, it's probably not that
>> important, but it was added to keep down the logging when using a
>> debug build as boot jdk, which has probably not been tested that much
>> either.
>
> LogVMOutput will only be turned on in a debug build if you also enable
> specific (mostly debug only) log/print/trace options.
I thought that these extra debug options were enabled by default in
debug build, and that required this action to disable them. Otherwise it
doesn't really make sense.

>
>> We should either fix this or remove the arguments completely.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193055
>> Patch inline:
>> diff --git a/make/autoconf/boot-jdk.m4 b/make/autoconf/boot-jdk.m4
>> --- a/make/autoconf/boot-jdk.m4
>> +++ b/make/autoconf/boot-jdk.m4
>> @@ -354,7 +354,7 @@
>>     AC_MSG_CHECKING([flags for boot jdk java command] )
>>
>>     # Disable special log output when a debug build is used as Boot
>> JDK...
>> -  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions
>> -XX:-UnlockDiagnosticVMOptions
>> -XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
>> +  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions
>> -XX:+UnlockDiagnosticVMOptions
>> -XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
>
> Fix is fine. But you could probably remove them too. Do you recall why
> this was added? It may relate to something now migrated to Unified
> Logging.
At first I thought that it was introduced as part of JDK-8010767, but
that only shuffled it around a bit. Going full Indiana Jones I've dug
all the way to the bottom of the repo. It was introduced by the very
first build-infra incarnation. :-)

So, I'd say, we should probably remove it instead. Updating my webrev:

diff --git a/make/autoconf/boot-jdk.m4 b/make/autoconf/boot-jdk.m4
--- a/make/autoconf/boot-jdk.m4
+++ b/make/autoconf/boot-jdk.m4
@@ -353,9 +353,6 @@

    AC_MSG_CHECKING([flags for boot jdk java command] )

-  # Disable special log output when a debug build is used as Boot JDK...
-  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions -XX:-UnlockDiagnosticVMOptions
-XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
-
    # Force en-US environment
    ADD_JVM_ARG_IF_OK([-Duser.language=en
-Duser.country=US],boot_jdk_jvmargs,[$JAVA])



/Magnus


>
> David
> -----
>
>>     # Force en-US environment
>>     ADD_JVM_ARG_IF_OK([-Duser.language=en
>> -Duser.country=US],boot_jdk_jvmargs,[$JAVA])
>>
>> /Magnus

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8193055 ADD_JVM_ARG_IF_OK always fails

David Holmes
On 5/12/2017 10:27 PM, Magnus Ihse Bursie wrote:

> On 2017-12-05 13:03, David Holmes wrote:
>> Hi Magnus,
>>
>> On 5/12/2017 8:49 PM, Magnus Ihse Bursie wrote:
>>> We mistakenly use -XX:-UnlockDiagnosticVMOptions instead of
>>> -XX:+UnlockDiagnosticVMOptions in a ADD_JVM_ARG_IF_OK call. This
>>> means that the test will always fail and the arguments will never be
>>> added to the command line.
>>>
>>> Since this has been the case all time along, it's probably not that
>>> important, but it was added to keep down the logging when using a
>>> debug build as boot jdk, which has probably not been tested that much
>>> either.
>>
>> LogVMOutput will only be turned on in a debug build if you also enable
>> specific (mostly debug only) log/print/trace options.
> I thought that these extra debug options were enabled by default in
> debug build, and that required this action to disable them. Otherwise it
> doesn't really make sense.

AFAICS that is not the case - you have to enable other flags explicitly
to implicitly enable LogVMOutput.

>>
>>> We should either fix this or remove the arguments completely.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193055
>>> Patch inline:
>>> diff --git a/make/autoconf/boot-jdk.m4 b/make/autoconf/boot-jdk.m4
>>> --- a/make/autoconf/boot-jdk.m4
>>> +++ b/make/autoconf/boot-jdk.m4
>>> @@ -354,7 +354,7 @@
>>>     AC_MSG_CHECKING([flags for boot jdk java command] )
>>>
>>>     # Disable special log output when a debug build is used as Boot
>>> JDK...
>>> -  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions
>>> -XX:-UnlockDiagnosticVMOptions
>>> -XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
>>> +  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions
>>> -XX:+UnlockDiagnosticVMOptions
>>> -XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
>>
>> Fix is fine. But you could probably remove them too. Do you recall why
>> this was added? It may relate to something now migrated to Unified
>> Logging.
> At first I thought that it was introduced as part of JDK-8010767, but
> that only shuffled it around a bit. Going full Indiana Jones I've dug
> all the way to the bottom of the repo. It was introduced by the very
> first build-infra incarnation. :-)
>
> So, I'd say, we should probably remove it instead. Updating my webrev:
>
> diff --git a/make/autoconf/boot-jdk.m4 b/make/autoconf/boot-jdk.m4
> --- a/make/autoconf/boot-jdk.m4
> +++ b/make/autoconf/boot-jdk.m4
> @@ -353,9 +353,6 @@
>
>     AC_MSG_CHECKING([flags for boot jdk java command] )
>
> -  # Disable special log output when a debug build is used as Boot JDK...
> -  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions -XX:-UnlockDiagnosticVMOptions
> -XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
> -
>     # Force en-US environment
>     ADD_JVM_ARG_IF_OK([-Duser.language=en
> -Duser.country=US],boot_jdk_jvmargs,[$JAVA])

Looks fine to me.

David
-----

>
>
> /Magnus
>
>
>>
>> David
>> -----
>>
>>>     # Force en-US environment
>>>     ADD_JVM_ARG_IF_OK([-Duser.language=en
>>> -Duser.country=US],boot_jdk_jvmargs,[$JAVA])
>>>
>>> /Magnus
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8193055 ADD_JVM_ARG_IF_OK always fails

Erik Joelsson
In reply to this post by Magnus Ihse Bursie
Looks good.

/Erik


On 2017-12-05 04:27, Magnus Ihse Bursie wrote:

> On 2017-12-05 13:03, David Holmes wrote:
>> Hi Magnus,
>>
>> On 5/12/2017 8:49 PM, Magnus Ihse Bursie wrote:
>>> We mistakenly use -XX:-UnlockDiagnosticVMOptions instead of
>>> -XX:+UnlockDiagnosticVMOptions in a ADD_JVM_ARG_IF_OK call. This
>>> means that the test will always fail and the arguments will never be
>>> added to the command line.
>>>
>>> Since this has been the case all time along, it's probably not that
>>> important, but it was added to keep down the logging when using a
>>> debug build as boot jdk, which has probably not been tested that
>>> much either.
>>
>> LogVMOutput will only be turned on in a debug build if you also
>> enable specific (mostly debug only) log/print/trace options.
> I thought that these extra debug options were enabled by default in
> debug build, and that required this action to disable them. Otherwise
> it doesn't really make sense.
>
>>
>>> We should either fix this or remove the arguments completely.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193055
>>> Patch inline:
>>> diff --git a/make/autoconf/boot-jdk.m4 b/make/autoconf/boot-jdk.m4
>>> --- a/make/autoconf/boot-jdk.m4
>>> +++ b/make/autoconf/boot-jdk.m4
>>> @@ -354,7 +354,7 @@
>>>     AC_MSG_CHECKING([flags for boot jdk java command] )
>>>
>>>     # Disable special log output when a debug build is used as Boot
>>> JDK...
>>> -  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions
>>> -XX:-UnlockDiagnosticVMOptions
>>> -XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
>>> +  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions
>>> -XX:+UnlockDiagnosticVMOptions
>>> -XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
>>
>> Fix is fine. But you could probably remove them too. Do you recall
>> why this was added? It may relate to something now migrated to
>> Unified Logging.
> At first I thought that it was introduced as part of JDK-8010767, but
> that only shuffled it around a bit. Going full Indiana Jones I've dug
> all the way to the bottom of the repo. It was introduced by the very
> first build-infra incarnation. :-)
>
> So, I'd say, we should probably remove it instead. Updating my webrev:
>
> diff --git a/make/autoconf/boot-jdk.m4 b/make/autoconf/boot-jdk.m4
> --- a/make/autoconf/boot-jdk.m4
> +++ b/make/autoconf/boot-jdk.m4
> @@ -353,9 +353,6 @@
>
>    AC_MSG_CHECKING([flags for boot jdk java command] )
>
> -  # Disable special log output when a debug build is used as Boot JDK...
> -  ADD_JVM_ARG_IF_OK([-XX:-PrintVMOptions
> -XX:-UnlockDiagnosticVMOptions
> -XX:-LogVMOutput],boot_jdk_jvmargs,[$JAVA])
> -
>    # Force en-US environment
>    ADD_JVM_ARG_IF_OK([-Duser.language=en
> -Duser.country=US],boot_jdk_jvmargs,[$JAVA])
>
>
>
> /Magnus
>
>
>>
>> David
>> -----
>>
>>>     # Force en-US environment
>>>     ADD_JVM_ARG_IF_OK([-Duser.language=en
>>> -Duser.country=US],boot_jdk_jvmargs,[$JAVA])
>>>
>>> /Magnus
>