<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>I think that's a good idea.</p>
<p>Thanks, Harold<br>
</p>
<div class="moz-cite-prefix">On 5/19/2020 11:49 AM,
<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:e118128e-a4dc-d5cd-0eac-b0a041f684fe@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<div class="moz-cite-prefix">Hi Harold and David,<br>
<br>
Just one comment.<br>
We could save on the CSR's for:<br>
 make/data/jdwp/jdwp.spec<br>
 <code></code>
src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java<br>
 <code></code>
src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java<br>
<br>
As we discussed with David, it could make sense to remove
duplication in the Serviceability class redefinition and
retransformation specs.<br>
I'd suggest something like this webrev:<br>
 <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/redef-spec-dedup.1/</a><br>
<br>
If the direction looks okay then I could file RFE+CSR (and post
an RFR).<br>
<br>
Thanks,<br>
Serguei<br>
 <br>
<br>
On 5/18/20 22:26, David Holmes wrote:<br>
</div>
<blockquote type="cite"
cite="mid:a2c2c36b-b1a2-eadc-b634-7aced81a2ae8@oracle.com">Hi
Harold, <br>
<br>
Adding serviceability-dev for the serviceability related
changes. <br>
<br>
Nit: "VM support for sealed classes" <br>
<br>
This RFR covers the VM, core-libs, serviceability and even some
langtools tests. AFAICS only the javac changes are not included
here so when and where will they be reviewed and under what bug
id? Ideally there will be a single JBS issue for "Implementation
of JEP 360: Sealed types". It's okay to break up the RFRs across
different areas, but it should be done under one bug id. <br>
<br>
Overall this looks good. I've looked at all files and mainly
have some style nits in various places. But there are some more
significant items below. <br>
<br>
On 14/05/2020 7:09 am, Harold Seigel wrote: <br>
<blockquote type="cite">Hi, <br>
<br>
Please review this patch for JVM and Core-libs support for the
JEP 360 Sealed Classes preview feature. Code changes include
the following: <br>
<br>
  * Processing of the new PermittedSubclasses attribute to
enforce that <br>
   a class or interface, whose super is a sealed class or
interface, <br>
   must be listed in the super's PermittedSubclasses
attribute. <br>
  * Disallow redefinition of a sealed class or interface if
its <br>
   redefinition would change its PermittedSubclasses
attribute. <br>
  * Support API's to determine if a class or interface is
sealed and, if <br>
   it's sealed, return a list of its permitted subclasses. <br>
  * asm support for the PermittedSubclasses attribute <br>
</blockquote>
<br>
I assume Remi is providing the upstream support in ASM? :) But
also see below ... <br>
<br>
<blockquote type="cite"> <br>
Open Webrev: <a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~vromero/8225056/webrev.00/index.html</a>
<br>
</blockquote>
<br>
make/data/jdwp/jdwp.spec <br>
<br>
There needs to be a sub-task and associated CSR request for this
JDWP spec update. I couldn't see this covered anywhere. <br>
<br>
--- <br>
<br>
src/hotspot/share/classfile/classFileParser.cpp <br>
<br>
3215 u2
ClassFileParser::parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs, <br>
3216Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â
const u1* const permitted_subclasses_attribute_start, <br>
3217Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â
TRAPS) { <br>
<br>
Indention on L3216/17 needs fixing after the copy'n'edit. <br>
<br>
3515Â Â return _major_version == JVM_CLASSFILE_MAJOR_VERSION
&& <br>
3516Â Â Â Â Â Â Â Â Â Â Â Â Â _minor_version == JAVA_PREVIEW_MINOR_VERSION
&& <br>
3517Â Â Â Â Â Â Â Â Â Â Â Â Â Arguments::enable_preview(); <br>
<br>
Too much indentation on L3516/17 <br>
<br>
3790Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â // Check for PermittedSubclasses tag <br>
<br>
That comment (copied from my nestmates code :) is in the wrong
place. It needs to be before <br>
<br>
3788Â Â Â Â Â Â Â Â Â Â Â Â if (tag ==
vmSymbols::tag_permitted_subclasses()) { <br>
<br>
<br>
Minor nit: I would suggest checking
parsed_permitted_subclasses_attribute before checking ACC_FINAL.
<br>
<br>
3876Â Â if (parsed_permitted_subclasses_attribute) { <br>
3877Â Â Â Â const u2 num_of_subclasses =
parse_classfile_permitted_subclasses_attribute( <br>
3878Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â cfs, <br>
3879 permitted_subclasses_attribute_start, <br>
3880Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â CHECK); <br>
<br>
Although it looks odd the preceding, similarly shaped, sections
all indent to the same absolute position. Can you make
L3878/78/80 match please. <br>
<br>
3882Â Â Â Â Â Â guarantee_property( <br>
3883Â Â Â Â Â Â Â Â permitted_subclasses_attribute_length == <br>
3884Â Â Â Â Â Â Â Â Â Â sizeof(num_of_subclasses) + sizeof(u2) *
num_of_subclasses, <br>
3885Â Â Â Â Â Â Â Â "Wrong PermittedSubclasses attribute length in
class file %s", CHECK); <br>
<br>
Nits: please reformat as: <br>
<br>
3882Â Â Â Â Â Â guarantee_property( <br>
3883Â Â Â Â Â Â Â Â permitted_subclasses_attribute_length ==
sizeof(num_of_subclasses) + sizeof(u2) * num_of_subclasses, <br>
3885Â Â Â Â Â Â Â Â "Wrong PermittedSubclasses attribute length in
class file %s", CHECK); <br>
<br>
It would also look slightly better if you shortened the name of
the num_of_subclasses variable. <br>
<br>
--- <br>
<br>
src/hotspot/share/classfile/classFileParser.hpp <br>
<br>
+Â Â u2 parse_classfile_permitted_subclasses_attribute(const
ClassFileStream* const cfs, <br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â const u1* const
permitted_subclasses_attribute_start, <br>
+Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â TRAPS); <br>
<br>
Please fix indentation after copy'n'edit. <br>
<br>
--- <br>
<br>
src/hotspot/share/oops/instanceKlass.cpp <br>
<br>
 247  if (classloader1 != classloader2) { <br>
<br>
I'm not clear what rule this is verifying. The same module check
follows this one. The rule is that the subclass must be
accessible to the superclass implying: <br>
1. same named module (regardless of class access modifiers); or
<br>
2. (implicitly in un-named module) same package if subclass not
public; or <br>
3. public subclass <br>
<br>
Having the same classloader implies same package, but that alone
doesn't address 2 or 3. So this doesn't conform to proposed JVMS
rules. <br>
<br>
<br>
 264    if (_constants->tag_at(cp_index).is_klass()) { <br>
 265      Klass* k2 = _constants->klass_at(cp_index,
CHECK_false); <br>
<br>
You've copied this code from the nestmember checks but your
changes don't quite make sense to me. If we have already checked
is_klass() then klass_at() cannot lead to any exceptions. <br>
<br>
 272      if (name == k->name()) { <br>
 273        log_trace(class, sealed)("- Found it at
permitted_subclasses[%d] => cp[%d]", i, cp_index); <br>
 274        return true; <br>
<br>
I was wondering why you don't resolve the cp entry when you find
the name matches, as we do for nest members, but realized that
unlike the nest membership check, which can happen many times
for a given class, this permitted subclass check can only happen
once per class. As you don't actually resolve here, and given
that the earlier check cannot throw exceptions, it follows that
the entire method never results in any exceptions and so callers
should not be using the CHECK macro. <br>
<br>
--- <br>
<br>
src/hotspot/share/oops/method.cpp <br>
<br>
I don't understand how knowing the class is sealed allows you to
infer that a non-final method is actually final ?? <br>
<br>
--- <br>
<br>
 src/hotspot/share/prims/jvm.cpp <br>
<br>
It would be simpler (and cheaper) if the Java side of this
ensures it doesn't call into the VM with an array or primitive
class. <br>
<br>
--- <br>
<br>
 src/hotspot/share/prims/jvmti.xml <br>
<br>
The JVM TI spec changes also need to be covered by a CSR
request. <br>
<br>
--- <br>
<br>
src/hotspot/share/prims/jvmtiRedefineClasses.cpp <br>
<br>
We should file a RFE to refactor the logic that checks that an
attribute consisting of a list of classes has not changed. :) <br>
<br>
Aside: I spotted a bug in the nest member code (missing NULL
check!) thanks to your change :) <br>
<br>
--- <br>
<br>
src/java.base/share/classes/java/lang/Class.java <br>
<br>
There needs to be a CSR request for these changes. <br>
<br>
+Â Â Â Â Â * Returns an array containing {@code ClassDesc} objects
representing all the <br>
+Â Â Â Â Â * permitted subclasses of this {@linkplain Class} if it
is sealed. Returns an empty array if this <br>
+Â Â Â Â Â * {@linkplain Class} is not sealed. <br>
<br>
should add "or this class represents an array or primitive type"
(using the standard wording for such cases). <br>
<br>
+Â Â Â Â Â * @throws IllegalArgumentException if a class descriptor
is not in the correct format <br>
<br>
IllegalArgumentException is not an appropriate exception to use
as this method takes no arguments. If the class descriptor is
not valid and it comes from the VM then I think we have a
problem with how the VM validates class descriptors. Any IAE
from ClassDesc.of should be caught and converted to a more
suitable exception type - preferably InternalError if the VM
should always return valid strings. <br>
<br>
+Â Â Â Â public ClassDesc[] getPermittedSubclasses() { <br>
<br>
As mentioned for jvm.cpp this Java code should do the isArray()
and isPrimitive() check before calling the VM. <br>
<br>
+Â Â Â Â Â Â Â Â String[] descriptors = getPermittedSubclasses0(); <br>
<br>
Nit: what you get from the VM are not descriptors, just name
strings in internal form. This wouldn't really matter except it
then looks strange to call ClassDesc.of(...) instead of
ClassDesc.ofDescriptor(...). <br>
<br>
+Â Â Â Â Â Â Â Â if (descriptors == null <br>
<br>
The VM never returns null. <br>
<br>
+Â Â Â Â Â Â Â Â return getPermittedSubclasses().length != 0; <br>
<br>
It's grossly inefficient to create the ClassDesc array and then
throw it away IMO. The result should be cached either in a field
of Class or in the ReflectionData of the class. <br>
<br>
--- <br>
<br>
src/java.base/share/classes/jdk/internal/org/objectweb/asm/ClassReader.java
<br>
<br>
!        // - The offset of the PermittedSubclasses attribute,
or 0 <br>
         int permittedSubtypesOffset = 0; <br>
<br>
Obviously ASM already has some prelim support for sealed
classes, but now that the attribute has been renamed that should
also flow through to the ASM code ie the variable, not just the
comment. <br>
<br>
Ditto for ClassWriter.java and its fields. <br>
<br>
--- <br>
<br>
src/java.base/share/native/libjava/Class.c <br>
<br>
     {"isRecord0",           "()Z",        (void
*)&JVM_IsRecord}, <br>
+    {"getPermittedSubclasses0", "()[" STR,   (void
*)&JVM_GetPermittedSubclasses}, <br>
<br>
please align (void <br>
<br>
--- <br>
<br>
src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
<br>
src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java <br>
<br>
There needs to be a CSR for these changes too. <br>
<br>
--- <br>
<br>
test/langtools/tools/javac/processing/model/TestSourceVersion.java <br>
<br>
!                   // Assume "record" and "sealed" will be
restricted keywords. <br>
!                   "record", "sealed"); <br>
<br>
What about the non-sealed keyword defined in the JEP? <br>
<br>
--- <br>
<br>
In the tests you don't need to explicitly include
sun.hotspot.WhiteBox$WhiteBoxPermission on the
ClassFileInstaller invocation. (previous RFE's have been
removing existing occurrences after the CFI was fixed to handle
it internally). <br>
<br>
Please ensure all new tests have an @bug 8225056 (or whatever
the actual JBS issue will be) <br>
<br>
All test classes (and thus files) should be named in camel-case
i.e. C1 not c1, C2 not c2, SuperClass not superClass etc. <br>
<br>
<br>
test/hotspot/jtreg/runtime/modules/sealedP1/superClass.jcod <br>
test/hotspot/jtreg/runtime/sealedClasses/Pkg/sealedInterface.jcod <br>
<br>
Please add comments clarifying why these must be jcod files. <br>
<br>
--- <br>
<br>
That's it from me. <br>
<br>
Thanks, <br>
David <br>
----- <br>
<br>
<br>
<br>
<blockquote type="cite">JBS bug: <a
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8225056"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8225056</a>
<br>
<br>
Java Language Spec changes:
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jls.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jls.html</a>
<br>
<br>
JVM Spec changes:
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jvms.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jvms.html</a>
<br>
<br>
JEP 360: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8227043"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8227043</a>
<br>
<br>
JVM CSR: <a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8242578"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8242578</a>
<br>
<br>
Changes to javac and other language tools will be reviewed
separately. <br>
<br>
Thanks, Harold <br>
<br>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</body>
</html>