[8u] RFR for JDK-8162419: closed/com/oracle/jfr/runtime/TestVMInfoEvent.sh failing after JDK-8155968
David Holmes
david.holmes at oracle.com
Mon Aug 15 01:39:15 UTC 2016
I am okay with this version as well.
Thanks,
David
On 13/08/2016 5:21 AM, Chris Plummer wrote:
> 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