[9] RFR (S): Class.getSimpleName() should work for non-JLS compliant class names (original) (raw)

Vladimir Ivanov vladimir.x.ivanov at oracle.com
Tue Apr 14 10:59:53 UTC 2015

David, John, thanks for review!

Best regards, Vladimir Ivanov

On 4/14/15 3:47 AM, David Holmes wrote:

On 14/04/2015 3:39 AM, Vladimir Ivanov wrote:

John, David, thanks for the feedback!

What do you think about the following version: http://cr.openjdk.java.net/~vlivanov/8057919/webrev.02/jdk http://cr.openjdk.java.net/~vlivanov/8057919/webrev.02/hotspot Looks good - thanks. David Best regards, Vladimir Ivanov On 4/10/15 10:15 PM, John Rose wrote: On Apr 9, 2015, at 4:16 PM, David Holmes <david.holmes at oracle.com_ _<mailto:david.holmes at oracle.com>> wrote:

Hi Vladimir, On 10/04/2015 12:51 AM, Vladimir Ivanov wrote: David, thanks for the feedback!

Updated webrev: http://cr.openjdk.java.net/~vlivanov/8057919/webrev.01/jdk http://cr.openjdk.java.net/~vlivanov/8057919/webrev.01/hotspot I restored original logic and call into VM only if it fails. This change seems fine to me, but I think John may prefer to see getSimpleName implemented such that it always returns the name from the innerclass attribute. (Though perhaps with caching if performance is a concern?) Yes, I do prefer the new logic instead of a combination of old and new. Two concerns: Somebody somewhere might be relying on a bug in the old logic and be frustrated by seeing that bug fixed. This is a fine line to walk, but in this case I think the behavior change (if any) will show up in places where we already have outages. Perhaps someone is post-processing the result of getSimpleName to correct it. If so they may no longer need to do that. Second, the new logic may itself have issues. A typical problem with the InnerClasses attribute is it is broken or stripped, or a related nestmate is missing. (See parallel thread in core-libs-dev on 8076264.) But the proposed change takes effect after a call to Class.getEnclosingClass, which uses InstanceKlass::computeenclosingclassimpl and accesses the same InnerClasses record. This leads me to a comment: The common code (two uses of InnerClassesIterator with klassnameatmatches) should be factored out. The factored subroutine should search out the self-record in the InnerClasses attribute. The logic is tricky enough that I'm uncomfortable with it being duplicated. Key issue: We don't want to load any classes doing this lookup. (For bonus points, factor the third use in that file, the one that looks not for self but for children-of-self. Add a boolean flag that varies the test.) Bottom line: The new logic should replace the old.

The logic to compute simple name (Class.getSimpleName()) for inner/nested/local classes is tightly coupled with Java naming scheme and sometimes fails for classes generated from non-Java code.

Meta-question: if this is non-Java code then what does/should "simpleName" even mean? "Returns the simple name of the underlying class as given in the source code." If there is no (java) source code does this have any meaning? Should it return "" ? My reading of the JVMS is that InnerClasses attribute can be freely used by non-Java compilers to store simple names for classes they generate. The current wording for innernameindex doesn't mention Java language. True, but it does refer to source code: "represents the original simple name of C, as given in the source code from which this class file was compiled. " which seems misplaced as we're discussing classes for which there is no source code as such. It could be non-Java source code. In any case, the indicated component of the "binary name" of the class is well-defined, whether or not it occurs in source code. Note also that classes might be generated by ASM (no source code per se) but we still have a reasonable expectation of reflecting on them, even though reflection is (mostly) defined in terms of source code constructs. Anyway it just flags to me that perhaps these Class methods need to be generalized a bit given the support for non-Java languages on the JVM. But that's for core-libs folk to decide. Yes, I think that's the issue. The multi-language folks (like me) would be disappointed to hear that we decided that non-Java languages don't have any needs here; they do. Thanks, — John Thanks, David Best regards, Vladimir Ivanov Instead of parsing class name and try to extract simple name based on JLS rules, the fix I propose is to use InnerClasses attribute from the class file. Simple name is already recorded there. > JVMS-4.7.6: The InnerClasses Attribute "innernameindex: If C is anonymous (JLS §15.9.5), the value of the innernameindex item must be zero. Otherwise, the value of the innernameindex item must be a valid index into the constantpool table, and the entry at that index must be a CONSTANTUtf8info structure (§4.4.7) that represents the original simple name of C, as given in the source code from which this class file was compiled."

Since I consider backporting the fix into 8u60, I'd like to hear opinions about backward compatibility of such change. As an alternative solution, I can restore original logic and consult InnerClasses attribute when class name parsing logic fails. I think I'd prefer to see the VM call only used as a fallback if the regular parsing fails. That would prevent any compatibility issues for the 8u backport (ignoring tests that deliberately generate invalidly named classes). Thanks, David Testing: regression test, jck-runtime/javalang, jdk/test/java/lang/ Thanks! Best regards, Vladimir Ivanov

More information about the hotspot-dev mailing list