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
