[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 05:21:35 UTC 2016
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");
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