RFR: JDK-8162340: Better class stream parsing

[David Holmes]

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.

---

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