RFR: JDK-8065773: JDI: UOE is not thrown, when redefineClasses changes a class modifier

[Gary Adams]

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.