[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:21:16 UTC 2016
Actually maybe best not to change it in that case. I thought I had
checked JDK9 and didn't see the assert, but I see it there now. I must
have been looking at the 8u version.
thanks,
Chris
On 8/12/16 12:16 PM, Shafi Ahmad wrote:
> Hi Chris,
>
> Thank you for reviewing it. I have just copied the assert from jdk9 code change. I will make the change "buffer != NULL".
>
> Regards,
> Shafi
>
>> -----Original Message-----
>> From: Chris Plummer
>> Sent: Saturday, August 13, 2016 12:38 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,
>>
>> 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