[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 07:12:45 UTC 2016
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