[8u] RFR for JDK-8162419: closed/com/oracle/jfr/runtime/TestVMInfoEvent.sh failing after JDK-8155968
Chris Plummer
chris.plummer at oracle.com
Fri Aug 12 19:08:04 UTC 2016
Hi Shafi,
I'm not so sure the assert you added is all that useful. If you are
going to keep it, please use "buffer != NULL". Testing for a NULL
pointer should be explicit. Other than that it looks fine. No need for
another webrev.
thanks,
Chris
On 8/12/16 11:18 AM, Shafi Ahmad wrote:
> Hi,
>
> Please find updated webrev link.
>
> http://cr.openjdk.java.net/~shshahma/8162419/webrev.01/
>
> Regards,
> Shafi
>
>> -----Original Message-----
>> From: Chris Plummer
>> Sent: Friday, August 12, 2016 12:43 PM
>> To: Shafi Ahmad; hotspot-runtime-dev at openjdk.java.net; David Holmes
>> Subject: Re: [8u] RFR for JDK-8162419:
>> closed/com/oracle/jfr/runtime/TestVMInfoEvent.sh failing after JDK-
>> 8155968
>>
>> On 8/11/16 10:21 PM, Shafi Ahmad wrote:
>>> Hi Chris,
>>>
>>> Thanks for reviewing.
>>>
>>>> -----Original Message-----
>>>> From: Chris Plummer
>>>> Sent: Friday, August 12, 2016 4:50 AM
>>>> To: Shafi Ahmad; hotspot-runtime-dev at openjdk.java.net; David Holmes
>>>> Subject: Re: [8u] RFR for JDK-8162419:
>>>> closed/com/oracle/jfr/runtime/TestVMInfoEvent.sh failing after JDK-
>>>> 8155968
>>>>
>>>> Hi Shafi,
>>>>
>>>> Please update the copyright date to 2016 and change "numbers of" to
>>>> "number of".
>>>>
>>>> I'm not so sure I agree with the comments in the CR that you can just
>>>> backport this change to vsnprintf(), but not the other changes in the
>>>> relevant changeset. For example:
>>>>
>>>> --- a/src/share/vm/runtime/java.cpp Mon Nov 03 11:34:13 2014 -0800
>>>> +++ b/src/share/vm/runtime/java.cpp Wed Oct 29 10:13:24 2014 +0100
>>>> @@ -705,25 +705,35 @@
>>>> }
>>>>
>>>> void JDK_Version::to_string(char* buffer, size_t buflen) const {
>>>> + assert(buffer && buflen > 0, "call with useful buffer");
>>>> size_t index = 0;
>>>> +
>>>> if (!is_valid()) {
>>>> jio_snprintf(buffer, buflen, "%s", "(uninitialized)");
>>>> } else if (is_partially_initialized()) {
>>>> jio_snprintf(buffer, buflen, "%s", "(uninitialized) pre-1.6.0");
>>>> } else {
>>>> - index += jio_snprintf(
>>>> + int rc = jio_snprintf(
>>>> &buffer[index], buflen - index, "%d.%d", _major, _minor);
>>>> + if (rc == -1) return;
>>>> + index += rc;
>>>> if (_micro > 0) {
>>>> - index += jio_snprintf(&buffer[index], buflen - index, ".%d", _micro);
>>>> + rc = jio_snprintf(&buffer[index], buflen - index, ".%d",
>>>> + _micro);
>>>> }
>>>>
>>>> I think your change to vsnprintf() will break
>>>> JDK_Version::to_string() if the above diff if not applied. You could
>>>> argue that the above code is already broken because -1 is could be
>> returned to it on Windows.
>>>> However, your changes expand that risk to all platforms.
>>> I am agree with you. I think I have to revisit at least all reference of
>> jio_snprintf for which we are using return value of this method.
>>> shafi at shafi-ahmad:~/Java/jdk8/jdk8u-dev/hotspot$ find ./ -name "*.cpp"
>>> -exec grep -H jio_snprintf {} \; | egrep "=|if" | grep -v close
>>> ./src/share/vm/ci/ciEnv.cpp: int ret = jio_snprintf(buffer, O_BUFLEN,
>>> "replay_pid%p_compid%d.log", os::current_process_id(), compile_id);
>>> ./src/share/vm/ci/ciEnv.cpp: int ret = jio_snprintf(buffer, O_BUFLEN,
>>> "inline_pid%p_compid%d.log", os::current_process_id(), compile_id);
>>> ./src/share/vm/runtime/os.cpp: const int printed = jio_snprintf(buffer,
>> buffer_length, iso8601_format,
>>> ./src/share/vm/runtime/arguments.cpp: int ret = jio_snprintf(b,
>> buf_sz, "%d", os::current_process_id());
>>> ./src/share/vm/runtime/arguments.cpp: // if jio_snprintf fails or the
>> buffer is not long enough to hold
>>> ./src/share/vm/runtime/java.cpp: index += jio_snprintf(
>>> ./src/share/vm/runtime/java.cpp: index += jio_snprintf(&buffer[index],
>> buflen - index, ".%d", _micro);
>>> ./src/share/vm/runtime/java.cpp: index += jio_snprintf(&buffer[index],
>> buflen - index, "_%02d", _update);
>>> ./src/share/vm/runtime/java.cpp: index += jio_snprintf(&buffer[index],
>> buflen - index, "%c", _special);
>>> ./src/share/vm/runtime/java.cpp: index += jio_snprintf(&buffer[index],
>> buflen - index, "-b%02d", _build);
>>> ./src/share/vm/runtime/deoptimization.cpp: len = jio_snprintf(buf,
>> buflen, "#%d", trap_state);
>>> ./src/share/vm/runtime/deoptimization.cpp: len = jio_snprintf(buf,
>> buflen, "%s%s",
>>> ./src/share/vm/runtime/deoptimization.cpp: len = jio_snprintf(buf,
>> buflen, "reason='%s' action='%s'",
>>> ./src/share/vm/runtime/deoptimization.cpp: len = jio_snprintf(buf,
>> buflen, "reason='%s' action='%s' index='%d'",
>>> ./src/share/vm/services/diagnosticArgument.cpp: jio_snprintf(buf,
>>> len, "%s", (c != NULL) ? c : "");
>>> ./src/share/vm/classfile/vmSymbols.cpp: int len = jio_snprintf(buf,
>>> buflen, "%s: %s%s.%s%s",
>>> ./src/share/vm/classfile/classLoader.cpp: if (jio_snprintf(path,
>> sizeof(path), "%s%s%s", _dir, os::file_separator(), name) == -1) {
>>> ./src/share/vm/classfile/verifier.cpp: jio_snprintf(message, message_len,
>> "Could not link verifier");
>>> ./src/share/vm/utilities/ostream.cpp: int result =
>> jio_snprintf(current_file_name, JVM_MAXPATHLEN,
>>> ./src/share/vm/utilities/ostream.cpp: int result =
>> jio_snprintf(current_file_name, JVM_MAXPATHLEN, "%s.%d"
>> CURRENTAPPX,
>>> ./src/share/vm/utilities/vmError.cpp: int n = jio_snprintf(buf, buflen,
>>> ./src/share/vm/utilities/vmError.cpp: int fsep_len =
>> jio_snprintf(&buf[pos], buflen-pos, "%s", os::file_separator());
>>> ./src/share/vm/utilities/vmError.cpp: int pos = jio_snprintf(buf, buflen,
>> "%s%s", tmpdir, os::file_separator());
>>> ./src/cpu/ppc/vm/methodHandles_ppc.cpp: jio_snprintf(buf, 100,
>> "verify_ref_kind expected %x", ref_kind);
>>> ./src/cpu/x86/vm/methodHandles_x86.cpp: jio_snprintf(buf, 100,
>> "verify_ref_kind expected %x", ref_kind);
>>> ./src/cpu/sparc/vm/methodHandles_sparc.cpp: jio_snprintf(buf, 100,
>> "verify_ref_kind expected %x", ref_kind);
>>> ./src/os/bsd/vm/os_bsd.cpp: int n = jio_snprintf(buffer, bufferSize,
>>> "/cores");
>> Hi Shafi,
>>
>> As David pointed out, it looks like only java.cpp needs to be updated to
>> account for changes you are making jio_snprintf. The others either don't use
>> the result (even if it is assigned to a local) or already have special handling for
>> -1. The exception is the os_bsd.cpp case. I noticed it looks buggy, both in
>> JDK9 and JDK8u.
>>
>> cheers,
>>
>> Chris
>>> I will resend the updated webrev.
>>>
>>> Jdk9:src/share/vm/runtime/java.cpp
>>> 714 int rc = jio_snprintf(
>>> 715 &buffer[index], buflen - index, "%d.%d", _major, _minor);
>>> 716 if (rc == -1) return;
>>> 717 index += rc;
>>> 718 if (_security > 0) {
>>> 719 rc = jio_snprintf(&buffer[index], buflen - index, ".%d", _security);
>>> 720 }
>>> 721 if (_patch > 0) {
>>> 722 rc = jio_snprintf(&buffer[index], buflen - index, ".%d", _patch);
>>> 723 if (rc == -1) return;
>>> 724 index += rc;
>>> 725 }
>>>
>>> After line# 719 we are not updating the index variable and hence if
>> _security > 0 and _patch > 0 then in that case value of _security is getting
>> overwritten by value of _patch in the buffer.
>>> Is this a bug or we are ignoring _security field, in that case this is redundant
>> code? Please note _security field is not there in jdk8 code.
>>> Regards,
>>> Shafi
>>>
>>>
>>>
>>>> cheers,
>>>>
>>>> Chris
>>>>
>>>> On 8/11/16 5:14 AM, Shafi Ahmad wrote:
>>>> > Hi,
>>>> >
>>>> > Could I get one more review for this safe change.
>>>> >
>>>> > Regards,
>>>> > Shafi
>>>> >
>>>> >> -----Original Message-----
>>>> >> From: David Holmes
>>>> >> Sent: Thursday, August 11, 2016 9:52 AM >> To: Shafi Ahmad;
>>>> hotspot- runtime-dev at openjdk.java.net
>>>> >> Subject: Re: [8u] RFR for JDK-8162419:
>>>> >> closed/com/oracle/jfr/runtime/TestVMInfoEvent.sh failing after
>>>> JDK- >>
>>>> 8155968 >> >> Hi Shafi, >> >> On 10/08/2016 6:34 PM, Shafi Ahmad
>> wrote:
>>>> >>> Hi,
>>>> >>>
>>>> >>> Please review the code change for "JDK-8162419:
>>>> >> closed/com/oracle/jfr/runtime/TestVMInfoEvent.sh failing after
>>>> JDK- >> 8155968" to jdk8u.
>>>> >>> Please note this is partial backport of >>
>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/110ec5963eb1#l23.1
>>>> >>> Summary:
>>>> >>> Microsoft version of vsnprintf() behaves differently from the
>>>> standard C
>>>>>> version when there is not enough space in the buffer.
>>>> >>> Microsoft version doesn't null terminates its output under
>>>> error conditions, >> whereas the standard C version does. On
>>>> Windows, it returns -1.
>>>> >>> We handle both cases here and always return -1, and perform
>>>> null >> termination.
>>>> >>
>>>> >> This looks fine to me.
>>>> >>
>>>> >> Thanks,
>>>> >> David
>>>> >>
>>>> >>> Jdk8 bug: https://bugs.openjdk.java.net/browse/JDK-8162419
>>>> >>> Webrev link:
>>>> http://cr.openjdk.java.net/~shshahma/8162419/webrev.00/
>>>> >>>
>>>> >>> Testing: jprt
>>>> >>>
>>>> >>> Regards,
>>>> >>> Shafi
>>>> >>>
>>>>
>>>>
More information about the hotspot-runtime-dev
mailing list