[8u] RFR for JDK-8162419: closed/com/oracle/jfr/runtime/TestVMInfoEvent.sh failing after JDK-8155968
Shafi Ahmad
shafi.s.ahmad at oracle.com
Fri Aug 12 19:16:45 UTC 2016
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