RFR: JDK-8162340: Better class stream parsing

[Coleen Phillimore]

On 7/22/16 2:04 AM, David Holmes wrote:
> Hi Karen,
>
> Generally this looks good. A couple of comments below ...
>
> On 22/07/2016 3:23 AM, Karen Kinnear wrote:
>> Please review:
>> https://bugs.openjdk.java.net/browse/JDK-8162340
>>
>> http://cr.openjdk.java.net/~acorn/8162340.hs/webrev/
>
> src/share/vm/classfile/systemDictionary.cpp
>
> 336   // leave PENDING_EXCEPTION handling in this code to ensure we do 
> the placeholder cleanup (find_and_remove)
>  337   // and do not throw exceptions while holding locks
>
> Nit: can you rebalance the lines, the first is rather long.
>
> It is unclear to me what this comment means given the following code 
> barely references PENDING_EXCEPTION - AFAICS just the end check at:
>
>  415   if (HAS_PENDING_EXCEPTION || superk_h() == NULL) {
>
> Does it really mean "Don't use CHECK_ macros to ensure we do the 
> placeholder cleanuop ..." ?
>
> Similarly with:
>
>  642 // leave PENDING_EXCEPTION handling in this code to ensure we do 
> the placeholder cleanup (find_and_remove)
>  643 // and do not throw exceptions while holding locks
>
> Though in that case we have:
>
> 722     k = SystemDictionary::handle_parallel_super_load(name, 
> superclassname,
>  723         class_loader, protection_domain, lockObject, THREAD);
>  724     if (HAS_PENDING_EXCEPTION) {
>  725       return NULL;
>
> in which case we could be using CHECK_NULL anyway.
>
> Similarly:
>
> 1665 // leave PENDING_EXCEPTION handling in this code to ensure we do 
> the placeholder cleanup (find_and_remove)
> 1666 // and do not throw exceptions while holding locks
>
> Again not clear what it refers to as we don't have the pattern "if 
> (!HAS_PENDING_EXCEPTION) ..." that was removed elsewhere. And we do 
> have some uses of check macros.

I thought about making this comment as well, and can I suggest that 
these comments be moved closer to the code they are trying to describe:

In resolve_super_or_fail, it should be before the call to resolve_or_null().

In resolve_instance_klass_or_null, it should be before the call to 
load_instance_klass().

Then they'll make sense and they won't get lost with the large comments, 
and the one before the function definition which isn't true.

Also, it's a bit of unfortunate API that the dictionary and placeholder 
functions take a last THREAD parameter since it looks like you're 
ignoring a pending exception, but they don't throw exceptions.   As a 
future cleanup, THREAD should be first there to eliminate this confusion.

Or add Thread* thread = THREAD; at the top and use this version for 
these calls.   As a future cleanup if you wish.

Thanks,
Coleen

>
> ---
>
> src/share/vm/oops/instanceKlass.cpp
>
> +  // ensure java/ packages only loaded by boot or platform builtin 
> loaders
>
> Nit: the platform loader need not be a "builtin" loader - can we drop 
> "builtin"?
>
>> http://cr.openjdk.java.net/~acorn/8162340.jdk.test/webrev
>
> Is there an existing test for non-anonymous classes that claim to be 
> in the java/ package hierarchy?
>
> Thanks,
> David
> -----
>
>> When moving the check from resolve_from_stream to set_package, I removed
>> the no longer need parsed_class (!) which I should have cleaned up when
>> tightening the placeholder table cleanup years ago.
>>
>> Tests run:
>> 1. updated VMAnonymousClass test
>> 2. jcks
>> 3. rbt hs-nightly-runtime linux-x64 in progress
>>
>> thanks,
>> Karen
>>
>>