Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading
Hi Serguei,
On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:
PING: This is pretty small and easy to review fix.
Thanks!
Serguei
On 5/19/20 09:28, serguei.spit...@oracle.com wrote:
Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/
Summary:
There are two places in the native part of test that cause assert
and WARNING with the -Xcheck:jni.
The assert is because there is no check for pending exception after
the call to:
jni->CallBooleanMethod(klass, is_hid_mid);
Using a JNI ExceptionCheck()after the call fixes the issue.
bool res = jni->CallBooleanMethod(klass, is_hid_mid);
if (jni->ExceptionCheck()) {
LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
}
return res;
That will fix the pending_jni_exception_check error, but if an exception
actually occurs what will be returned? And whatever is returned, the
callers of this method don't themselves check for pending exceptions so
they will treat it as if the exception didn't occur - at least until we
finally return to Java code. Perhaps any exception should result in
jni->FatalError as happens with any JVMTI error?
The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, &count, &loader_classes);
produces the warning (with a java level stack trace): WARNING: JNI
local refs: 94, exceeds capacity: 32
It is because the GetClassLoaderClasses returns an array of local
references to the loader classes.
Using a JNI EnsureLocalCapacity() before the JVM TI call also fixes
the issue.
The warning suggests using 1024 is a bit of overkill. :)
Cheers,
David
Testing:
Running the test test/hotspot/jtreg/serviceability/jvmti/HiddenClass
locally.
Will run a mach5 job as well.
Thanks,
Serguei
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi David, Some tiers in Mach5 are configured to run tests with '-showversion' VM options. In JDK-8242009 [3] we started forwarding test VM options to j-*tools and some tests that launch them and expect an empty stderr (apart from VM warnings) need to be corrected to either ignore version messages in the output or to ensure that -showversion VM option is not passed to j*-tool even if it is passed to the test itself. Best regards, Daniil On 5/21/20, 10:41 PM, "David Holmes" wrote: Hi Dannil, On 22/05/2020 3:06 pm, Daniil Titov wrote: > Please review a webrev [1] that reverts the changes done in jdk.test.lib.process.OutputAnalyzer in [3]. > > Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings . The current webrev [1] reverts this change Thank you for reverting the OutputAnalyzer change. > and instead makes the tests that expects an empty stderr from launched j-* tools to filter out '-showversion' from test options when forwarding test VM option to j*-tools. That seems workable, though I'm unclear what is adding the "-showversion" in the first place? Thanks, David - > Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests are in progress. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01 > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993 > [3] https://bugs.openjdk.java.net/browse/JDK-8242009 > > Thank you, > Daniil > >
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Daniil, On 22/05/2020 5:24 pm, Daniil Titov wrote: Hi David, Some tiers in Mach5 are configured to run tests with '-showversion' VM options. In JDK-8242009 [3] we started forwarding test VM options to j-*tools Okay. Filtering it out seems fine then. Thanks, David and some tests that launch them and expect an empty stderr (apart from VM warnings) need to be corrected to either ignore version messages in the output or to ensure that -showversion VM option is not passed to j*-tool even if it is passed to the test itself. Best regards, Daniil On 5/21/20, 10:41 PM, "David Holmes" wrote: Hi Dannil, On 22/05/2020 3:06 pm, Daniil Titov wrote: > Please review a webrev [1] that reverts the changes done in jdk.test.lib.process.OutputAnalyzer in [3]. > > Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings . The current webrev [1] reverts this change Thank you for reverting the OutputAnalyzer change. > and instead makes the tests that expects an empty stderr from launched j-* tools to filter out '-showversion' from test options when forwarding test VM option to j*-tools. That seems workable, though I'm unclear what is adding the "-showversion" in the first place? Thanks, David - > Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests are in progress. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01 > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993 > [3] https://bugs.openjdk.java.net/browse/JDK-8242009 > > Thank you, > Daniil > >
Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading
Hi David,
Thank you for the comments!
On 5/21/20 23:58, David Holmes wrote:
Hi Serguei,
On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:
PING: This is pretty small and easy to review fix.
Thanks!
Serguei
On 5/19/20 09:28, serguei.spit...@oracle.com wrote:
Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/
Summary:
There are two places in the native part of test that cause assert
and WARNING with the -Xcheck:jni.
The assert is because there is no check for pending exception
after the call to:
jni->CallBooleanMethod(klass, is_hid_mid);
Using a JNI ExceptionCheck()after the call fixes the issue.
bool res = jni->CallBooleanMethod(klass, is_hid_mid);
if (jni->ExceptionCheck()) {
LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
}
return res;
That will fix the pending_jni_exception_check error, but if an
exception actually occurs what will be returned? And whatever is
returned, the callers of this method don't themselves check for
pending exceptions so they will treat it as if the exception didn't
occur - at least until we finally return to Java code. Perhaps any
exception should result in jni->FatalError as happens with any JVMTI
error?
You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception details.
The exception can be printed before call to FatalError.
The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, &count, &loader_classes);
produces the warning (with a java level stack trace): WARNING: JNI
local refs: 94, exceeds capacity: 32
It is because the GetClassLoaderClasses returns an array of local
references to the loader classes.
Using a JNI EnsureLocalCapacity() before the JVM TI call also
fixes the issue.
The warning suggests using 1024 is a bit of overkill. :)
What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.
Thanks!
Serguei
Cheers,
David
Testing:
Running the test
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.
Will run a mach5 job as well.
Thanks,
Serguei
Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading
Hi David,
The updated webrev is with your comments addressed:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.2/
Thanks,
Serguei
On 5/22/20 00:43, serguei.spit...@oracle.com wrote:
Hi David,
Thank you for the comments!
On 5/21/20 23:58, David Holmes wrote:
Hi Serguei,
On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:
PING: This is pretty small and easy to review fix.
Thanks!
Serguei
On 5/19/20 09:28, serguei.spit...@oracle.com wrote:
Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/
Summary:
There are two places in the native part of test that cause assert
and WARNING with the -Xcheck:jni.
The assert is because there is no check for pending exception
after the call to:
jni->CallBooleanMethod(klass, is_hid_mid);
Using a JNI ExceptionCheck()after the call fixes the issue.
bool res = jni->CallBooleanMethod(klass, is_hid_mid);
if (jni->ExceptionCheck()) {
LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
}
return res;
That will fix the pending_jni_exception_check error, but if an
exception actually occurs what will be returned? And whatever is
returned, the callers of this method don't themselves check for
pending exceptions so they will treat it as if the exception didn't
occur - at least until we finally return to Java code. Perhaps any
exception should result in jni->FatalError as happens with any JVMTI
error?
You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception
details.
The exception can be printed before call to FatalError.
The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, &count, &loader_classes);
produces the warning (with a java level stack trace): WARNING:
JNI local refs: 94, exceeds capacity: 32
It is because the GetClassLoaderClasses returns an array of local
references to the loader classes.
Using a JNI EnsureLocalCapacity() before the JVM TI call also
fixes the issue.
The warning suggests using 1024 is a bit of overkill. :)
What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.
Thanks!
Serguei
Cheers,
David
Testing:
Running the test
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.
Will run a mach5 job as well.
Thanks,
Serguei
Re: RFR: JDK-8225056 VM support for sealed classes
On 5/21/2020 2:33 PM, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
Hi Harold,
I think this webrev looks good! A couple of minor comments:
- oops/instanceKlass.cpp:
line #236, do you need a ResourceMark here? I noticed there is one
above at line #229 for the log_trace call that uses external_name().
- prims/jvmtiRedefineClasses.cpp:
line #878, I think you need a ResourceMark for this method as well if
you invoke the external_name for the log_trace calls and for
NEW_RESOURCE_ARRAY_RETURN_NULL()?
Tests look good.
Thanks,
Lois
This webrev contains the following significant changes:
1. The format/indentation issues in classFileParser.cpp were fixed.
2. Unneeded checks in InstanceKlass::has_as_permitted_subclass() were
removed and the TRAPS parameter was removed.
3. The changes to klassVtable.* and method.* were reverted. Those
changes were from when sealed classes were marked as final, and are
no longer valid.
4. Method getPermittedSubclasses() in Class.java was renamed to
permittedSubclasses() and its implementation was changed.
5. Variables and methods for 'asm' were renamed from
'permittedSubtypes' to 'permittedSubclasses'.
6. Classes for sealed classes tests were changed to start with capital
letters.
7. Changes to langtools tests were removed from this webrev. They are
covered by the javac webrev (JDK-8244556).
8. The CSR's for JVMTI, JDWP, and JDI are in progress.
Please also see comments inline.
On 5/19/2020 1:26 AM, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
The javac changes are being reviewed as bug JDK-8227406. We
understand the need to do the reviews under one bug.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this JDWP
spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong place.
It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong PermittedSubclasses attribute length in class
file %s", CHECK);
Nits: please reformat as:
3882 guarantee_property(
3883
Re: RFR: JDK-8225056 VM support for sealed classes
Thanks Lois!
I'll add the two ResourceMarks before the changes get pushed.
Harold
On 5/22/2020 11:07 AM, Lois Foltan wrote:
On 5/21/2020 2:33 PM, Harold Seigel wrote:
Hi David,
Thanks for looking at this! Please review this new webrev:
http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/
Hi Harold,
I think this webrev looks good! A couple of minor comments:
- oops/instanceKlass.cpp:
line #236, do you need a ResourceMark here? I noticed there is one
above at line #229 for the log_trace call that uses external_name().
- prims/jvmtiRedefineClasses.cpp:
line #878, I think you need a ResourceMark for this method as well
if you invoke the external_name for the log_trace calls and for
NEW_RESOURCE_ARRAY_RETURN_NULL()?
Tests look good.
Thanks,
Lois
This webrev contains the following significant changes:
1. The format/indentation issues in classFileParser.cpp were fixed.
2. Unneeded checks in InstanceKlass::has_as_permitted_subclass() were
removed and the TRAPS parameter was removed.
3. The changes to klassVtable.* and method.* were reverted. Those
changes were from when sealed classes were marked as final, and are
no longer valid.
4. Method getPermittedSubclasses() in Class.java was renamed to
permittedSubclasses() and its implementation was changed.
5. Variables and methods for 'asm' were renamed from
'permittedSubtypes' to 'permittedSubclasses'.
6. Classes for sealed classes tests were changed to start with capital
letters.
7. Changes to langtools tests were removed from this webrev. They are
covered by the javac webrev (JDK-8244556).
8. The CSR's for JVMTI, JDWP, and JDI are in progress.
Please also see comments inline.
On 5/19/2020 1:26 AM, David Holmes wrote:
Hi Harold,
Adding serviceability-dev for the serviceability related changes.
Nit: "VM support for sealed classes"
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included here
so when and where will they be reviewed and under what bug id?
Ideally there will be a single JBS issue for "Implementation of JEP
360: Sealed types". It's okay to break up the RFRs across different
areas, but it should be done under one bug id.
The javac changes are being reviewed as bug JDK-8227406. We
understand the need to do the reviews under one bug.
Overall this looks good. I've looked at all files and mainly have
some style nits in various places. But there are some more
significant items below.
On 14/05/2020 7:09 am, Harold Seigel wrote:
Hi,
Please review this patch for JVM and Core-libs support for the JEP
360 Sealed Classes preview feature. Code changes include the
following:
* Processing of the new PermittedSubclasses attribute to enforce
that
a class or interface, whose super is a sealed class or interface,
must be listed in the super's PermittedSubclasses attribute.
* Disallow redefinition of a sealed class or interface if its
redefinition would change its PermittedSubclasses attribute.
* Support API's to determine if a class or interface is sealed
and, if
it's sealed, return a list of its permitted subclasses.
* asm support for the PermittedSubclasses attribute
I assume Remi is providing the upstream support in ASM? :) But also
see below ...
Open Webrev:
http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html
make/data/jdwp/jdwp.spec
There needs to be a sub-task and associated CSR request for this
JDWP spec update. I couldn't see this covered anywhere.
---
src/hotspot/share/classfile/classFileParser.cpp
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs,
3216 const u1* const permitted_subclasses_attribute_start,
3217 TRAPS) {
Indention on L3216/17 needs fixing after the copy'n'edit.
3515 return _major_version == JVM_CLASSFILE_MAJOR_VERSION &&
3516 _minor_version == JAVA_PREVIEW_MINOR_VERSION &&
3517 Arguments::enable_preview();
Too much indentation on L3516/17
3790 // Check for PermittedSubclasses tag
That comment (copied from my nestmates code :) is in the wrong
place. It needs to be before
3788 if (tag == vmSymbols::tag_permitted_subclasses()) {
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
3876 if (parsed_permitted_subclasses_attribute) {
3877 const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute(
3878 cfs,
3879 permitted_subclasses_attribute_start,
3880 CHECK);
Although it looks odd the preceding, similarly shaped, sections all
indent to the same absolute position. Can you make L3878/78/80 match
please.
3882 guarantee_property(
3883 permitted_subclasses_attribute_length ==
3884 sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses,
3885 "Wrong
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Hi Daniil, I'm not sure all this retry logic is a good way. As mentioned in jira the most important part of the testing is ensuring that you find all the created threads when they are alive, and you don't find them when they are dead. The actual thread count checking is not that important. I agree with this and I'd just simplify the test by removing checks for thread count. VM may create and destroy internal threads when it needs it. --alex On 05/18/2020 10:31, Daniil Titov wrote: Please review the change [1] that fixes an intermittent failure of the test. This test creates and destroys a given number of daemon/user threads and validates the count of those started/stopped threads against values returned from ThreadMXBean thread counts. The problem here is that if some internal threads is started ( e.g. " HotSpotGraalManagement Bean Registration"), or destroyed (e.g. "JVMCI CompilerThread ") the test hangs waiting for expected number of live threads. The fix limits the time the test is waiting for desired number of live threads and in case if this limit is exceeded the test repeats itself. Testing. Test with Graal on and Mach5 tier1-tier7 test passed. [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8131745 Thank you, Daniil
RFR: JDK-8244703: "platform encoding not initialized" exceptions with debugger, JNI
Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8244703 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/jdwp_javalib_dep/webrev/ The issue is a regression from JDK-8222529 which introduced dependency jdwp lib of java lib. The fix removes the dependency and implements platform to utf8 conversion using existing jdwp code. --alex
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
Hi Alex, I was thinking about it as well. But the test also indirectly tests getTotalStartedThreadCount(), getThreadCount(), and getPeakThreadCount() methods of ThreadMXBean. So I tried to not reduce the test coverage. Best regards, Daniil On 5/22/20, 10:26 AM, "Alex Menkov" wrote: Hi Daniil, I'm not sure all this retry logic is a good way. As mentioned in jira the most important part of the testing is ensuring that you find all the created threads when they are alive, and you don't find them when they are dead. The actual thread count checking is not that important. I agree with this and I'd just simplify the test by removing checks for thread count. VM may create and destroy internal threads when it needs it. --alex On 05/18/2020 10:31, Daniil Titov wrote: > Please review the change [1] that fixes an intermittent failure of the test. > > This test creates and destroys a given number of daemon/user threads and validates the count of those started/stopped threads against values returned from ThreadMXBean thread counts. The problem here is that if some internal threads is started ( e.g. " HotSpotGraalManagement Bean Registration"), or destroyed (e.g. "JVMCI CompilerThread ") the test hangs waiting for expected number of live threads. > > The fix limits the time the test is waiting for desired number of live threads and in case if this limit is exceeded the test repeats itself. > > Testing. Test with Graal on and Mach5 tier1-tier7 test passed. > > [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.01 > [2] https://bugs.openjdk.java.net/browse/JDK-8131745 > > Thank you, > Daniil > > >
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
I don't think this approach adds any value to test coverage. We have other tests which are targeted to test the methods. IMO the test can still test the methods, but with relaxed conditions. maybe something like 1. at the beginning: getTotalStartedThreadCount() >= 1, getThreadCount() >= 1 getPeakThreadCount() >= 1 2. when threads are alive: getTotalStartedThreadCount() >= ALL_THREADS + value from step 1, getThreadCount() >= 1 + ALL_THREADS getPeakThreadCount() >= 1 + ALL_THREADS 3. after the threads terminates: getThreadCount() >= 1 getTotalStartedThreadCount() and getPeakThreadCount() >= values at step 2 --alex On 05/22/2020 10:51, Daniil Titov wrote: Hi Alex, I was thinking about it as well. But the test also indirectly tests getTotalStartedThreadCount(), getThreadCount(), and getPeakThreadCount() methods of ThreadMXBean. So I tried to not reduce the test coverage. Best regards, Daniil On 5/22/20, 10:26 AM, "Alex Menkov" wrote: Hi Daniil, I'm not sure all this retry logic is a good way. As mentioned in jira the most important part of the testing is ensuring that you find all the created threads when they are alive, and you don't find them when they are dead. The actual thread count checking is not that important. I agree with this and I'd just simplify the test by removing checks for thread count. VM may create and destroy internal threads when it needs it. --alex On 05/18/2020 10:31, Daniil Titov wrote: > Please review the change [1] that fixes an intermittent failure of the test. > > This test creates and destroys a given number of daemon/user threads and validates the count of those started/stopped threads against values returned from ThreadMXBean thread counts. The problem here is that if some internal threads is started ( e.g. " HotSpotGraalManagement Bean Registration"), or destroyed (e.g. "JVMCI CompilerThread ") the test hangs waiting for expected number of live threads. > > The fix limits the time the test is waiting for desired number of live threads and in case if this limit is exceeded the test repeats itself. > > Testing. Test with Graal on and Mach5 tier1-tier7 test passed. > > [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.01 > [2] https://bugs.openjdk.java.net/browse/JDK-8131745 > > Thank you, > Daniil > > >
Re: RFR: JDK-8244703: "platform encoding not initialized" exceptions with debugger, JNI
On 22/05/2020 18:50, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8244703 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/jdwp_javalib_dep/webrev/ The issue is a regression from JDK-8222529 which introduced dependency jdwp lib of java lib. The fix removes the dependency and implements platform to utf8 conversion using existing jdwp code. This looks good to me. While we are in the area, can you look at printLastError in transport.c? I'm just wondering if parentheses could be added to "len+len/2+2" to make it clear how maxlen is set. -Alan.
Re: RFR: JDK-8244703: "platform encoding not initialized" exceptions with debugger, JNI
Hi Alan, thank you for the review. On 05/22/2020 11:16, Alan Bateman wrote: On 22/05/2020 18:50, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8244703 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/jdwp_javalib_dep/webrev/ The issue is a regression from JDK-8222529 which introduced dependency jdwp lib of java lib. The fix removes the dependency and implements platform to utf8 conversion using existing jdwp code. This looks good to me. While we are in the area, can you look at printLastError in transport.c? I'm just wondering if parentheses could be added to "len+len/2+2" to make it clear how maxlen is set. Not sure I understand what parentheses do you mean. And I'm not sure why len+len/2+2 is used there. In my fix I allocated len*4+1 (for the worst case - each symbol requires 4 bytes to encode plus 1 byte for terminal 0). --alex -Alan.
Re: RFR (XS): 8245392: Remove duplication in class redefinition and retransformation specs
Hi Serguei, Just one very minor editing suggestion. Where you have "If canUnrestrictedlyRedefineClasses() is false," there is a comma placed here, but the previous two bullet items are of a similar form, yet have no comma. I suggest you make all 3 consistent. I think either with or without a comma is acceptable in this case. From what I read if the opening phrase is 4 or fewer words, the comma is optional. I'm just suggesting you be consistent. thanks, Chris On 5/21/20 10:02 PM, serguei.spit...@oracle.com wrote: Please, review a fix for: https://bugs.openjdk.java.net/browse/JDK-8245392 CSR draft (one CSR reviewer is needed before finalizing it): https://bugs.openjdk.java.net/browse/JDK-8245433 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/src/ Updated specs: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/api/java.instrument/java/lang/instrument/Instrumentation.html http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/specs/jdwp/jdwp-protocol.html http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/api/jdk.jdi/com/sun/jdi/VirtualMachine.html Summary: The fix is to replace in Instrumentation, JDI and JDWP spec a description of class redefinition or retransformation restriction with a link to the supporting JVM TI function where it has been already documented. This spec refactoring should help in cases when new 'unmodifiable in redefinition' class file attributes are added. Testing: Built docs and checked the link works as expected. Thanks, Serguei
Re: RFR (XS): 8245392: Remove duplication in class redefinition and retransformation specs
Hi Chris, Thank you for the review! I'll make it consistent. Thanks, Serguei On 5/22/20 11:33, Chris Plummer wrote: Hi Serguei, Just one very minor editing suggestion. Where you have "If canUnrestrictedlyRedefineClasses() is false," there is a comma placed here, but the previous two bullet items are of a similar form, yet have no comma. I suggest you make all 3 consistent. I think either with or without a comma is acceptable in this case. From what I read if the opening phrase is 4 or fewer words, the comma is optional. I'm just suggesting you be consistent. thanks, Chris On 5/21/20 10:02 PM, serguei.spit...@oracle.com wrote: Please, review a fix for: https://bugs.openjdk.java.net/browse/JDK-8245392 CSR draft (one CSR reviewer is needed before finalizing it): https://bugs.openjdk.java.net/browse/JDK-8245433 Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/src/ Updated specs: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/api/java.instrument/java/lang/instrument/Instrumentation.html http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/specs/jdwp/jdwp-protocol.html http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/docs/api/jdk.jdi/com/sun/jdi/VirtualMachine.html Summary: The fix is to replace in Instrumentation, JDI and JDWP spec a description of class redefinition or retransformation restriction with a link to the supporting JVM TI function where it has been already documented. This spec refactoring should help in cases when new 'unmodifiable in redefinition' class file attributes are added. Testing: Built docs and checked the link works as expected. Thanks, Serguei
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
Hi Daniil,
There is one reference to "jvmwarningmsg" that occurs before it is
declared while all the rest all come after. It probably would make sense
to move its declaration up near the top of the file.
92 private static void matchListedProcesses(OutputAnalyzer output) {
93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX)
94 .stderrShouldBeEmptyIgnoreVMWarnings();
95 }
We probably use this coding pattern all over the place, but I think it
just leads to less readable code. Any reason not to use:
92 private static void matchListedProcesses(OutputAnalyzer output) {
93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX);
94 output.stderrShouldBeEmptyIgnoreVMWarnings();
95 }
I just don't see the point of the chaining, and don't understand why all
these OutputAnalyzer methods return the "this" object in the first
place. Maybe I'm missing something. I guess maybe there are cases where
the OutputAnalyzer object is not already in a local variable, adding
some value to the chaining, but that's not the case here, and I think if
it were the case it would be more readable just to stick the
OutputAnalyzer object in a local. There one other case of this:
154 private static void matchPerfCounters(OutputAnalyzer output) {
155 output.stdoutShouldMatchByLine(PERF_COUNTER_REGEX,null,
156 PERF_COUNTER_REGEX)
157 .stderrShouldBeEmptyIgnoreVMWarnings();
158 }
I think you can also add spaces after the commas, and probably make the
first stdoutShouldMatchByLine() one line.
thanks,
Chris
On 5/21/20 10:06 PM, Daniil Titov wrote:
Please review a webrev [1] that reverts the changes done in
jdk.test.lib.process.OutputAnalyzer in [3].
Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings()
methods to ignore also VM version strings . The current webrev [1] reverts this
change and instead makes the tests that expects an empty stderr from launched
j-* tools to filter out '-showversion' from test options when forwarding test
VM option to j*-tools.
Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests are in
progress.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993
[3] https://bugs.openjdk.java.net/browse/JDK-8242009
Thank you,
Daniil
Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading
Hi Serguei,
Looks good, and I agree with David's comments. I was thinking the same
thing when I first looked at your original changes.
thanks,
Chris
On 5/22/20 2:32 AM, serguei.spit...@oracle.com wrote:
Hi David,
The updated webrev is with your comments addressed:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.2/
Thanks,
Serguei
On 5/22/20 00:43, serguei.spit...@oracle.com wrote:
Hi David,
Thank you for the comments!
On 5/21/20 23:58, David Holmes wrote:
Hi Serguei,
On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:
PING: This is pretty small and easy to review fix.
Thanks!
Serguei
On 5/19/20 09:28, serguei.spit...@oracle.com wrote:
Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/
Summary:
There are two places in the native part of test that cause
assert and WARNING with the -Xcheck:jni.
The assert is because there is no check for pending exception
after the call to:
jni->CallBooleanMethod(klass, is_hid_mid);
Using a JNI ExceptionCheck()after the call fixes the issue.
bool res = jni->CallBooleanMethod(klass, is_hid_mid);
if (jni->ExceptionCheck()) {
LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
}
return res;
That will fix the pending_jni_exception_check error, but if an
exception actually occurs what will be returned? And whatever is
returned, the callers of this method don't themselves check for
pending exceptions so they will treat it as if the exception didn't
occur - at least until we finally return to Java code. Perhaps any
exception should result in jni->FatalError as happens with any JVMTI
error?
You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception
details.
The exception can be printed before call to FatalError.
The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, &count, &loader_classes);
produces the warning (with a java level stack trace): WARNING:
JNI local refs: 94, exceeds capacity: 32
It is because the GetClassLoaderClasses returns an array of
local references to the loader classes.
Using a JNI EnsureLocalCapacity() before the JVM TI call also
fixes the issue.
The warning suggests using 1024 is a bit of overkill. :)
What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.
Thanks!
Serguei
Cheers,
David
Testing:
Running the test
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.
Will run a mach5 job as well.
Thanks,
Serguei
Re: PING: Re: RFR (XS): 8244571: assert(!_thread->is_pending_jni_exception_check()) failed: Pending JNI Exception Check during class loading
Thank you for the review, Chris!
Serguei
On 5/22/20 11:57, Chris Plummer wrote:
Hi Serguei,
Looks good, and I agree with David's comments. I was thinking the same
thing when I first looked at your original changes.
thanks,
Chris
On 5/22/20 2:32 AM, serguei.spit...@oracle.com wrote:
Hi David,
The updated webrev is with your comments addressed:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.2/
Thanks,
Serguei
On 5/22/20 00:43, serguei.spit...@oracle.com wrote:
Hi David,
Thank you for the comments!
On 5/21/20 23:58, David Holmes wrote:
Hi Serguei,
On 22/05/2020 4:17 pm, serguei.spit...@oracle.com wrote:
PING: This is pretty small and easy to review fix.
Thanks!
Serguei
On 5/19/20 09:28, serguei.spit...@oracle.com wrote:
Please, review fix for:
https://bugs.openjdk.java.net/browse/JDK-8244571
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/8244571-jvmti-test-jnicheck.1/
Summary:
There are two places in the native part of test that cause
assert and WARNING with the -Xcheck:jni.
The assert is because there is no check for pending exception
after the call to:
jni->CallBooleanMethod(klass, is_hid_mid);
Using a JNI ExceptionCheck()after the call fixes the issue.
bool res = jni->CallBooleanMethod(klass, is_hid_mid);
if (jni->ExceptionCheck()) {
LOG0("is_hidden: Exception in jni CallBooleanMethod\n");
}
return res;
That will fix the pending_jni_exception_check error, but if an
exception actually occurs what will be returned? And whatever is
returned, the callers of this method don't themselves check for
pending exceptions so they will treat it as if the exception didn't
occur - at least until we finally return to Java code. Perhaps any
exception should result in jni->FatalError as happens with any
JVMTI error?
You are right, it would be more clean to call jni->FatalError.
I was also thinking about it but also worried to get the exception
details.
The exception can be printed before call to FatalError.
The following call to the JVM TI function:
err = jvmti->GetClassLoaderClasses(loader, &count, &loader_classes);
produces the warning (with a java level stack trace): WARNING:
JNI local refs: 94, exceeds capacity: 32
It is because the GetClassLoaderClasses returns an array of
local references to the loader classes.
Using a JNI EnsureLocalCapacity() before the JVM TI call also
fixes the issue.
The warning suggests using 1024 is a bit of overkill. :)
What capacity would be more reasonable, 256 or 512?
Let's pick 256. This is just a warning, the test is still passing.
Thanks!
Serguei
Cheers,
David
Testing:
Running the test
test/hotspot/jtreg/serviceability/jvmti/HiddenClass locally.
Will run a mach5 job as well.
Thanks,
Serguei
Re: RFR: 8131745: java/lang/management/ThreadMXBean/AllThreadIds.java still fails intermittently
I was thinking in a similar direction. It is better to count only tested threads instead of the unreliable and expensive retries. Thanks, Serguei On 5/22/20 10:26, Alex Menkov wrote: Hi Daniil, I'm not sure all this retry logic is a good way. As mentioned in jira the most important part of the testing is ensuring that you find all the created threads when they are alive, and you don't find them when they are dead. The actual thread count checking is not that important. I agree with this and I'd just simplify the test by removing checks for thread count. VM may create and destroy internal threads when it needs it. --alex On 05/18/2020 10:31, Daniil Titov wrote: Please review the change [1] that fixes an intermittent failure of the test. This test creates and destroys a given number of daemon/user threads and validates the count of those started/stopped threads against values returned from ThreadMXBean thread counts. The problem here is that if some internal threads is started ( e.g. " HotSpotGraalManagement Bean Registration"), or destroyed (e.g. "JVMCI CompilerThread ") the test hangs waiting for expected number of live threads. The fix limits the time the test is waiting for desired number of live threads and in case if this limit is exceeded the test repeats itself. Testing. Test with Graal on and Mach5 tier1-tier7 test passed. [1] http://cr.openjdk.java.net/~dtitov/8131745/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8131745 Thank you, Daniil
Re: RFR: JDK-8244703: "platform encoding not initialized" exceptions with debugger, JNI
Hi Alex, The fix looks good. > And I'm not sure why len+len/2+2 is used there. > In my fix I allocated len*4+1 (for the worst case - each symbol requires 4 bytes to encode plus 1 byte for terminal 0). I agree, the size len+len/2+2 looks very strange. Most likely, we get error messages truncated. Should we replace it with len * sizeof(int) + 1 ? Thanks, Serguei On 5/22/20 11:33, Alex Menkov wrote: Hi Alan, thank you for the review. On 05/22/2020 11:16, Alan Bateman wrote: On 22/05/2020 18:50, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8244703 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/jdwp_javalib_dep/webrev/ The issue is a regression from JDK-8222529 which introduced dependency jdwp lib of java lib. The fix removes the dependency and implements platform to utf8 conversion using existing jdwp code. This looks good to me. While we are in the area, can you look at printLastError in transport.c? I'm just wondering if parentheses could be added to "len+len/2+2" to make it clear how maxlen is set. Not sure I understand what parentheses do you mean. And I'm not sure why len+len/2+2 is used there. In my fix I allocated len*4+1 (for the worst case - each symbol requires 4 bytes to encode plus 1 byte for terminal 0). --alex -Alan.
RFR: JDK-8245660: Try to recover from "Windbg Error: WaitForEvent failed" error
Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8245660 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/windbg_waitForEvent_try/webrev/ This is temporary change to try different approaches to fix https://bugs.openjdk.java.net/browse/JDK-8204994 (SA might fail to attach to process with "Windbg Error: WaitForEvent failed") --alex
Re: RFR: JDK-8245660: Try to recover from "Windbg Error: WaitForEvent failed" error
Hi Alex, I think this is a good experiment, but I don't really see a reason to push the change and wait for a failure to pop up in CI testing. I think you should be able to get the data you are looking for with adhoc runs. I find it pretty easy to reproduce by just running all the SA tests about 100 times (maybe more). In fact it was so noisy for me when I was trying to reproduce some very rare x86 failures that I stopped doing the testing on windows and just stuck with osx and linux. Is there a reason the 2nd AttachProcess() does not re-fetch ptrIDebugControl_ID? Is it sure to be the same value as with the first attach? thanks, Chris On 5/22/20 3:51 PM, Alex Menkov wrote: Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8245660 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/windbg_waitForEvent_try/webrev/ This is temporary change to try different approaches to fix https://bugs.openjdk.java.net/browse/JDK-8204994 (SA might fail to attach to process with "Windbg Error: WaitForEvent failed") --alex
Re: RFR: JDK-8244703: "platform encoding not initialized" exceptions with debugger, JNI
Hi Serguei, thanks for the review. On 05/22/2020 12:47, serguei.spit...@oracle.com wrote: Hi Alex, The fix looks good. > And I'm not sure why len+len/2+2 is used there. > In my fix I allocated len*4+1 (for the worst case - each symbol requires 4 bytes to encode plus 1 byte for terminal 0). I agree, the size len+len/2+2 looks very strange. Most likely, we get error messages truncated. I suppose usually system error messages are in English (i.e. each symbol requires only 1 byte in utf8), so no truncation occurs. Should we replace it with len * sizeof(int) + 1 ? size of utf8 string does not depend on sizeof(int). Per RFC each symbol can be encoded by 1..4 byte(s). Maybe Alan can explain this len+len/2+2 value. --alex Thanks, Serguei On 5/22/20 11:33, Alex Menkov wrote: Hi Alan, thank you for the review. On 05/22/2020 11:16, Alan Bateman wrote: On 22/05/2020 18:50, Alex Menkov wrote: Hi all, please review the fix for https://bugs.openjdk.java.net/browse/JDK-8244703 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/jdwp_javalib_dep/webrev/ The issue is a regression from JDK-8222529 which introduced dependency jdwp lib of java lib. The fix removes the dependency and implements platform to utf8 conversion using existing jdwp code. This looks good to me. While we are in the area, can you look at printLastError in transport.c? I'm just wondering if parentheses could be added to "len+len/2+2" to make it clear how maxlen is set. Not sure I understand what parentheses do you mean. And I'm not sure why len+len/2+2 is used there. In my fix I allocated len*4+1 (for the worst case - each symbol requires 4 bytes to encode plus 1 byte for terminal 0). --alex -Alan.
Re: RFR: JDK-8245660: Try to recover from "Windbg Error: WaitForEvent failed" error
Hi Chris, On 05/22/2020 17:12, Chris Plummer wrote: Hi Alex, I think this is a good experiment, but I don't really see a reason to push the change and wait for a failure to pop up in CI testing. I think you should be able to get the data you are looking for with adhoc runs. I find it pretty easy to reproduce by just running all the SA tests about 100 times (maybe more). In fact it was so noisy for me when I was trying to reproduce some very rare x86 failures that I stopped doing the testing on windows and just stuck with osx and linux. I thought it's quite hard to reproduce the issue. But I'll try to run several times. Is there a reason the 2nd AttachProcess() does not re-fetch ptrIDebugControl_ID? Is it sure to be the same value as with the first attach? It's just a native pointer saved in java field (initialized during debugger COM object creation). No reason to re-fetch it. --alex thanks, Chris On 5/22/20 3:51 PM, Alex Menkov wrote: Hi all, Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8245660 webrev: http://cr.openjdk.java.net/~amenkov/jdk15/windbg_waitForEvent_try/webrev/ This is temporary change to try different approaches to fix https://bugs.openjdk.java.net/browse/JDK-8204994 (SA might fail to attach to process with "Windbg Error: WaitForEvent failed") --alex
