RFR 8218705: Test sun/tools/jcmd/TestJcmdDefaults.java fails on Linux
Please review the change that fix the test failure. The problem here is that the code in sun.tools.common.ProcessArgumentMatcher.check() method was supposed to fallback to the default mechanism for retrieving the main class name if sun.tools.common.ProcessHelper (recently introduced in JDK-8205654) failed to detect it, but it does not. The change fixes that problem. Webrev: http://cr.openjdk.java.net/~dtitov/8218705/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8218705 Thanks! --Daniil
Re: RFR: JDK-8065773: JDI: UOE is not thrown, when redefineClasses changes a class modifier
Gary, I have filed https://bugs.openjdk.java.net/browse/JDK-8218703 against JVM TI. But in doing so I realized these current test changes are actually still incorrect. The problem is that these tests are changing the modifiers on nested types and in that case it is the inner_class_access_flags that JVM TI should be examining (which can encode all four access levels), not the top-level access flag (which only handles public or not-public). This is all rather a mess. David - On 9/02/2019 7:48 am, David Holmes wrote: On 8/02/2019 11:38 pm, Gary Adams wrote: On 2/5/19, 4:59 PM, David Holmes wrote: On 5/02/2019 10:17 pm, Gary Adams wrote: On 2/4/19, 8:04 PM, David Holmes wrote: Hi Gary, On 5/02/2019 12:01 am, Gary Adams wrote: Two of the redefine classes tests (021, 023) have been on the ProblemList since they were first brought into the open repos. Both tests made an incorrect assumption about the access modifiers in class files. There is a single bit in the binary class file that defines if an interface is public or not public (See JVMS Table 4.1-B ACC_PUBLIC). When the test classes are compiled for use in the redefine operation, if a source used public or protected the ACC_PUBLIC bit was set. Several modifiers are used in these tests to confirm UnsupportedOperationException is thrown or not thrown. This proposed change simply corrects the expected results according to the JVM Specification. Webrev: http://cr.openjdk.java.net/~gadams/8065773/webrev.00/index.html test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021.java 62 * Test cases 3 (private) and 4 (static) map to not public, so will "static" is not related to access modifiers so the ACC_PUBLIC bit is not relevant. "static" can only be applied to (nested) member types and is represented by the ACC_STATIC bit as per JVMS "Table 4.7.6-A. Nested class access and property flags". What this testcase does is add "static" to a nested interface and expects JVM TI to check if the class modifier has changed. But the "static" is not encoded in the class "access flags", but in the inner class attribute (inner_class_access_flags). To me this testcase should throw UOE, but JVM TI is not checking the right flags. David I think the test is demonstrating that static does not change the ACC_PUBLIC bit. And what is that a demonstration of? static has nothing to do with access control so if it did change the ACC_PUBLIC bit we'd have a serious issue. This changeset does not change the handling of that testcase. The test is adding a new class modifier "static" which it originally expected to trigger a UOE but it doesn't because JVM TI is not checking for that kind of change correctly. You modified the test to no longer expect UOE so that it passes but: a) that's wrong as it shouldn't pass; and b) it's now documented as passing because it doesn't change the public-ness of the class but that's completely irrelevant. So your change to the static testcase is not correct and should not be applied. David - I think you misread the webrev. The 4th test case was already being allowed to not throw UOE. The change I proposed allows the 3rd testcase to not throw UOE. e.g. redefining package to private accessibility is still not ACC_PUBLIC. Okay in regards to your changeset this comment is misleading for the static case (public/not-public is not the issue as we aren't changing an access modified - changing the static modifier simply leaves the class modifiers unchanged): 62 * Test cases 3 (private) and 4 (static) map to not public, so will 63 * not throw UnsupportedOperationException during the redefine 64 * class operation. but the whole test is wrong for the "static" case and JVM TI is broken for the static case so I'll file a new bug to get JVM TI and the test fixed. David
Re: RFR: JDK-8068225: nsk/jdi/EventQueue/remove_l/remove_l005 intermittently times out
Hi Gary, Looks good to me too :) Thanks, Jc On Fri, Feb 8, 2019 at 10:40 AM serguei.spit...@oracle.com < serguei.spit...@oracle.com> wrote: > Hi Gary, > > I'm Okay with the fix if the test run is good. > > Thanks, > Serguei > > > On 2/8/19 05:25, Gary Adams wrote: > > The intermittent failures for remove_l005 appears to be due to a > > timing issue > > in the shutdown handling after the test has completed. The debugger > > has sent > > a "quit" command, but the debugee has not completed processing the > > command > > and exited as expected. > > > > This proposed change modifies the Debugee.endDebugee() processing to > > wait for the debuggee to complete before doing the tear down processing > > on the debugger side of the connection. This sequencing did expose > > an issue in the canBeModified test, where the debugee was never resumed > > after the VMStart event was detected. Resuming the debugee allows it to > > respond as expected when the direct call to endDebugee is invoked. > > > > Testing is still in progress, but it looks like this will resolve a > > number of > > tail end intermittent failures. > > > > Issue: https://bugs.openjdk.java.net/browse/JDK-8068225 > > Webrev: http://cr.openjdk.java.net/~gadams/8068225/webrev.00/ > > -- Thanks, Jc
Re: RFR: JDK-8065773: JDI: UOE is not thrown, when redefineClasses changes a class modifier
On 8/02/2019 11:38 pm, Gary Adams wrote: On 2/5/19, 4:59 PM, David Holmes wrote: On 5/02/2019 10:17 pm, Gary Adams wrote: On 2/4/19, 8:04 PM, David Holmes wrote: Hi Gary, On 5/02/2019 12:01 am, Gary Adams wrote: Two of the redefine classes tests (021, 023) have been on the ProblemList since they were first brought into the open repos. Both tests made an incorrect assumption about the access modifiers in class files. There is a single bit in the binary class file that defines if an interface is public or not public (See JVMS Table 4.1-B ACC_PUBLIC). When the test classes are compiled for use in the redefine operation, if a source used public or protected the ACC_PUBLIC bit was set. Several modifiers are used in these tests to confirm UnsupportedOperationException is thrown or not thrown. This proposed change simply corrects the expected results according to the JVM Specification. Webrev: http://cr.openjdk.java.net/~gadams/8065773/webrev.00/index.html test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021.java 62 * Test cases 3 (private) and 4 (static) map to not public, so will "static" is not related to access modifiers so the ACC_PUBLIC bit is not relevant. "static" can only be applied to (nested) member types and is represented by the ACC_STATIC bit as per JVMS "Table 4.7.6-A. Nested class access and property flags". What this testcase does is add "static" to a nested interface and expects JVM TI to check if the class modifier has changed. But the "static" is not encoded in the class "access flags", but in the inner class attribute (inner_class_access_flags). To me this testcase should throw UOE, but JVM TI is not checking the right flags. David I think the test is demonstrating that static does not change the ACC_PUBLIC bit. And what is that a demonstration of? static has nothing to do with access control so if it did change the ACC_PUBLIC bit we'd have a serious issue. This changeset does not change the handling of that testcase. The test is adding a new class modifier "static" which it originally expected to trigger a UOE but it doesn't because JVM TI is not checking for that kind of change correctly. You modified the test to no longer expect UOE so that it passes but: a) that's wrong as it shouldn't pass; and b) it's now documented as passing because it doesn't change the public-ness of the class but that's completely irrelevant. So your change to the static testcase is not correct and should not be applied. David - I think you misread the webrev. The 4th test case was already being allowed to not throw UOE. The change I proposed allows the 3rd testcase to not throw UOE. e.g. redefining package to private accessibility is still not ACC_PUBLIC. Okay in regards to your changeset this comment is misleading for the static case (public/not-public is not the issue as we aren't changing an access modified - changing the static modifier simply leaves the class modifiers unchanged): 62 * Test cases 3 (private) and 4 (static) map to not public, so will 63 * not throw UnsupportedOperationException during the redefine 64 * class operation. but the whole test is wrong for the "static" case and JVM TI is broken for the static case so I'll file a new bug to get JVM TI and the test fixed. David
Re: RFR: JDK-8068225: nsk/jdi/EventQueue/remove_l/remove_l005 intermittently times out
Hi Gary, I'm Okay with the fix if the test run is good. Thanks, Serguei On 2/8/19 05:25, Gary Adams wrote: The intermittent failures for remove_l005 appears to be due to a timing issue in the shutdown handling after the test has completed. The debugger has sent a "quit" command, but the debugee has not completed processing the command and exited as expected. This proposed change modifies the Debugee.endDebugee() processing to wait for the debuggee to complete before doing the tear down processing on the debugger side of the connection. This sequencing did expose an issue in the canBeModified test, where the debugee was never resumed after the VMStart event was detected. Resuming the debugee allows it to respond as expected when the direct call to endDebugee is invoked. Testing is still in progress, but it looks like this will resolve a number of tail end intermittent failures. Issue: https://bugs.openjdk.java.net/browse/JDK-8068225 Webrev: http://cr.openjdk.java.net/~gadams/8068225/webrev.00/
Re: RFR: JDK-8065773: JDI: UOE is not thrown, when redefineClasses changes a class modifier
On 2/5/19, 4:59 PM, David Holmes wrote: On 5/02/2019 10:17 pm, Gary Adams wrote: On 2/4/19, 8:04 PM, David Holmes wrote: Hi Gary, On 5/02/2019 12:01 am, Gary Adams wrote: Two of the redefine classes tests (021, 023) have been on the ProblemList since they were first brought into the open repos. Both tests made an incorrect assumption about the access modifiers in class files. There is a single bit in the binary class file that defines if an interface is public or not public (See JVMS Table 4.1-B ACC_PUBLIC). When the test classes are compiled for use in the redefine operation, if a source used public or protected the ACC_PUBLIC bit was set. Several modifiers are used in these tests to confirm UnsupportedOperationException is thrown or not thrown. This proposed change simply corrects the expected results according to the JVM Specification. Webrev: http://cr.openjdk.java.net/~gadams/8065773/webrev.00/index.html test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021.java 62 * Test cases 3 (private) and 4 (static) map to not public, so will "static" is not related to access modifiers so the ACC_PUBLIC bit is not relevant. "static" can only be applied to (nested) member types and is represented by the ACC_STATIC bit as per JVMS "Table 4.7.6-A. Nested class access and property flags". What this testcase does is add "static" to a nested interface and expects JVM TI to check if the class modifier has changed. But the "static" is not encoded in the class "access flags", but in the inner class attribute (inner_class_access_flags). To me this testcase should throw UOE, but JVM TI is not checking the right flags. David I think the test is demonstrating that static does not change the ACC_PUBLIC bit. And what is that a demonstration of? static has nothing to do with access control so if it did change the ACC_PUBLIC bit we'd have a serious issue. This changeset does not change the handling of that testcase. The test is adding a new class modifier "static" which it originally expected to trigger a UOE but it doesn't because JVM TI is not checking for that kind of change correctly. You modified the test to no longer expect UOE so that it passes but: a) that's wrong as it shouldn't pass; and b) it's now documented as passing because it doesn't change the public-ness of the class but that's completely irrelevant. So your change to the static testcase is not correct and should not be applied. David - I think you misread the webrev. The 4th test case was already being allowed to not throw UOE. The change I proposed allows the 3rd testcase to not throw UOE. e.g. redefining package to private accessibility is still not ACC_PUBLIC.
RFR: JDK-8068225: nsk/jdi/EventQueue/remove_l/remove_l005 intermittently times out
The intermittent failures for remove_l005 appears to be due to a timing issue in the shutdown handling after the test has completed. The debugger has sent a "quit" command, but the debugee has not completed processing the command and exited as expected. This proposed change modifies the Debugee.endDebugee() processing to wait for the debuggee to complete before doing the tear down processing on the debugger side of the connection. This sequencing did expose an issue in the canBeModified test, where the debugee was never resumed after the VMStart event was detected. Resuming the debugee allows it to respond as expected when the direct call to endDebugee is invoked. Testing is still in progress, but it looks like this will resolve a number of tail end intermittent failures. Issue: https://bugs.openjdk.java.net/browse/JDK-8068225 Webrev: http://cr.openjdk.java.net/~gadams/8068225/webrev.00/
