Re: RFR: JDK-8225056 VM support for sealed classes
Hi Harold, On 22/05/2020 4:33 am, Harold Seigel wrote: Hi David, Thanks for looking at this! Please review this new webrev: http://cr.openjdk.java.net/~hseigel/webrev.01/webrev/ I'll list all relevant commens here rather than interspersing below so that it is easier to track. Mostly nits below, other than the is_permitted_subclass check in the VM, and the use of ReflectionData in java.lang.Class. -- src/hotspot/share/classfile/classFileParser.cpp + bool ClassFileParser::supports_sealed_types() { + return _major_version == JVM_CLASSFILE_MAJOR_VERSION && + _minor_version == JAVA_PREVIEW_MINOR_VERSION && + Arguments::enable_preview(); + } Nowe there is too little indentation - the subclauses of the conjunction expression should align[1] + bool ClassFileParser::supports_sealed_types() { + return _major_version == JVM_CLASSFILE_MAJOR_VERSION && + _minor_version == JAVA_PREVIEW_MINOR_VERSION && + Arguments::enable_preview(); + } 3791 if (parsed_permitted_subclasses_attribute) { 3792 classfile_parse_error("Multiple PermittedSubclasses attributes in class file %s", CHECK); 3793 // Classes marked ACC_FINAL cannot have a PermittedSubclasses attribute. 3794 } else if (_access_flags.is_final()) { 3795 classfile_parse_error("PermittedSubclasses attribute in final class file %s", CHECK); 3796 } else { 3797 parsed_permitted_subclasses_attribute = true; 3798 } The indent of the comment at L3793 is wrong, and its placement is awkward because it relates to the next condition. But we don't have to use if-else here as any parse error results in immediate return due to the CHECK macro. So the above can be reformatted as: 3791 if (parsed_permitted_subclasses_attribute) { 3792 classfile_parse_error("Multiple PermittedSubclasses attributes in class file %s", CHECK); 3793 } 3794 // Classes marked ACC_FINAL cannot have a PermittedSubclasses attribute. 3795 if (_access_flags.is_final()) { 3796 classfile_parse_error("PermittedSubclasses attribute in final class file %s", CHECK); 3797 } 3798 parsed_permitted_subclasses_attribute = true; --- src/hotspot/share/oops/instanceKlass.cpp The logic in InstanceKlass::has_as_permitted_subclass still does not implement the rules specified in the JVMS. It only implements a "same module" check, whereas the JVMS specifies an accessibility requirement as well. 730 bool InstanceKlass::is_sealed() const { 731 return _permitted_subclasses != NULL && 732 _permitted_subclasses != Universe::the_empty_short_array() && 733 _permitted_subclasses->length() > 0; 734 } Please align subclauses. --- src/hotspot/share/prims/jvm.cpp 2159 objArrayHandle result (THREAD, r); Please remove space after "result". As we will always create and return an arry, if you reverse these two statements: 2156 if (length != 0) { 2157 objArrayOop r = oopFactory::new_objArray(SystemDictionary::String_klass(), 2158length, CHECK_NULL); and these two: 2169 return (jobjectArray)JNIHandles::make_local(THREAD, result()); 2170 } then you can delete 2172 // if it gets to here return an empty array, cases will be: the class is primitive, or an array, or just not sealed 2173 objArrayOop result = oopFactory::new_objArray(SystemDictionary::String_klass(), 0, CHECK_NULL); 2174 return (jobjectArray)JNIHandles::make_local(env, result); The comment there is no longer accurate anyway. --- src/hotspot/share/prims/jvmtiRedefineClasses.cpp 857 static jvmtiError check_permitted_subclasses_attribute(InstanceKlass* the_class, 858 InstanceKlass* scratch_class) { Please align. --- src/hotspot/share/prims/jvmtiRedefineClasses.cpp 2007 if (permitted_subclasses != NULL) { permitted_subclasses cannot be NULL. I initially thought the bug was in the nest_members version of this code, but they both have the same properties: the member is initialized to NULL when the InstanceKlass is constructed, and set to either the proper array or the empty_array() when classfile parsing is complete. So redefinition cannot encounter a NULL value here. --- src/java.base/share/classes/java/lang/Class.java The use of ReflectionData is not correctly implemented. The ReflectionData instance is not constant but can be replaced when class redefinition operates. So you cannot do this: if (rd.permittedSubclasses != null) { return rd.permittedSubclasses; } because you may be returning the permittedSubclasses field of a different Reflectiondata instance. You need to read the field once into a local and thereafter use it. Similarly with:
Re: RFR: 8244993: Revert changes to OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() that allow version strings
On 24/05/2020 3:06 am, Chris Plummer wrote: On 5/23/20 6:03 AM, David Holmes wrote: Hi Chris, On 23/05/2020 4:50 am, Chris Plummer wrote: Hi Daniil, There is one reference to "jvmwarningmsg" that occurs before it is declared while all the rest all come after. It probably would make sense to move its declaration up near the top of the file. 92 private static void matchListedProcesses(OutputAnalyzer output) { 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX) 94 .stderrShouldBeEmptyIgnoreVMWarnings(); 95 } We probably use this coding pattern all over the place, but I think it just leads to less readable code. Any reason not to use: 92 private static void matchListedProcesses(OutputAnalyzer output) { 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX); 94 output.stderrShouldBeEmptyIgnoreVMWarnings(); 95 } I just don't see the point of the chaining, and don't understand why all these OutputAnalyzer methods return the "this" object in the first place. Maybe I'm missing something. They return "this" precisely so that you can chain. The API was designed for a style that allows: output.shouldContain(x).shouldNotContain(y).shouldContain(z) ... to avoid the repetition of "output". Yeah, I get that, but I never did like this pattern. I just don't find it as readable. For one, there's no conveyance of the method return type, not just because of the chaining, but also because the method name does not imply a return type. Chaining like getMethod().getClass().getName() is fine, because there are implied return types in the method names, and they clearly are being called for the purpose of returning a type. But when the return type is there solely for the purpose of chaining, it's not as obvious what is going on. Your example is easier to read because the method names are short, readily identified as related, and you made them all fit on one line with shortened arguments. Which is really an anti-pattern for this style of API :) That's not the case with Daniil's code. I just don't see the argument for saying that: 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX) 94 .stderrShouldBeEmptyIgnoreVMWarnings(); Note the '.' should line up Is somehow better than: 93 output.stdoutShouldMatchByLine(JCMD_LIST_REGEX); 94 output.stderrShouldBeEmptyIgnoreVMWarnings(); I don't have to look twice at the second version (or be familiar with the APIs being used) to know what's going on. All a matter of personal preference. :) Cheers, David Chris David - I guess maybe there are cases where the OutputAnalyzer object is not already in a local variable, adding some value to the chaining, but that's not the case here, and I think if it were the case it would be more readable just to stick the OutputAnalyzer object in a local. There one other case of this: 154 private static void matchPerfCounters(OutputAnalyzer output) { 155 output.stdoutShouldMatchByLine(PERF_COUNTER_REGEX,null, 156 PERF_COUNTER_REGEX) 157 .stderrShouldBeEmptyIgnoreVMWarnings(); 158 } I think you can also add spaces after the commas, and probably make the first stdoutShouldMatchByLine() one line. thanks, Chris On 5/21/20 10:06 PM, Daniil Titov wrote: Please review a webrev [1] that reverts the changes done in jdk.test.lib.process.OutputAnalyzer in [3]. Change [3] modified OutputAnalyzer stderrShouldBeEmptyIgnoreVMWarnings() methods to ignore also VM version strings . The current webrev [1] reverts this change and instead makes the tests that expects an empty stderr from launched j-* tools to filter out '-showversion' from test options when forwarding test VM option to j*-tools. Testing: Mach5 tier1-tier5 tests passed successfully. Tier6-tier7 tests are in progress. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8244993/webrev.01 [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8244993 [3] https://bugs.openjdk.java.net/browse/JDK-8242009 Thank you, Daniil