[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 18:18:13 UTC 2016
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