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

[gary.adams at oracle.com]

I'm willing to do the leg work on cleaning up the tests, but I'll need 
some help
identifying where the redefine operations are missing the 
inner_class_flags checks.

Is spec change or a CSR required when we change the current behavior to 
be more strict?

On 2/8/19 9:24 PM, David Holmes wrote:
> 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