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

[David Holmes]

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
-----