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

[David Holmes]

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 <code>UnsupportedOperationException</code> 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