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

[David Holmes]

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