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
>