RFR: JDK-8162340: Better class stream parsing

[Karen Kinnear]

Coleen & David,

Thank you for the review comments. 

I clarified the comments in all three methods to the appropriate match on:
// Be careful when modifying this code: once you have run placeholders()->find_and_add(PlaceholderTable::LOAD_SUPER),
// you need to find_and_remove it before returning. So be careful to not exit with a CHECK_ macro betweeen these calls.

Let me know if you want to see an updated web rev.

> On Jul 22, 2016, at 2:04 AM, David Holmes <david.holmes at oracle.com> 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

See comment about for response - which hopefully is much clearer.
I fixed the comment, left it at the top since people should be in the habit of reading global comments for a function even if they might
miss some embedded comments.  I left THREAD at the end since if you are not within the find_and_add/find_and_remove  you can use the
CHECK_ macro.
> ---
> 
> 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”?
We have a new term for JDK9 - we have three builtin loaders: boot, platform (formerly known as extension) and application.
So this platform loader must be a builtin loader.

Not to be confused with the system class loader which is the default delegation parent for custom class loaders.

thanks,
Karen

> 
>> 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
>> 
>>