RFR:7199207 Placeholders cleanup

[Karen Kinnear]

Coleen,

Now I see what you were saying. I could put the circularity error earlier and assert that
load_instance_added was false. That would be around 746 (after I release the lock). I will do that.
That is much simpler.

thanks for the suggestion,
Karen

On Jan 9, 2013, at 11:17 AM, Coleen Phillimore wrote:

> 
> Karen,
> 
> In systemDictionary, why can't you throw circularity error sooner, like around line 726 (after you release the lock)?
> 
> 811   // must throw error outside of owning lock
> 812   if (throw_circularity_error) {
> 813     ResourceMark rm(THREAD);
> 814     THROW_MSG_0(vmSymbols::java_lang_ClassCircularityError(), name->as_C_string());
> 815   }
> 
> I guess this compiles but it should be THROW_MSG_NULL() instead of THROW_MSG_0().   gcc is likely to be picky about things like this in the future.
> 
> There are some weird indentation around some of your changes, not caused by them, but could you clean up nearby indentation problems in systemDictionary?   Line 367, 352-356 (should be initialization all in one line).   I didn't see any others.  They are small cosmetic changes.
> 
> This change is great!   Thank you for taking the time to clean this up.  I can see this preventing all sorts of bugs in the future!
> 
> Coleen
> 
> On 1/4/2013 3:54 PM, Karen Kinnear wrote:
>> Please review:
>> http://cr.openjdk.java.net/~acorn/7199207.fix2merge/webrev/
>> 
>> bug: NPG: Crash in PlaceholderTable::verify, after a StackOverflow during class loading
>> https://jbs.oracle.com/bugs/browse/JDK-7199207
>> 
>> Thanks to Stefan Karlsson for the initial analysis and the test case.
>> 
>> Thanks to Coleen's suggestion, I've simplified the placeholder table entry cleanup.
>> Specifically the placeholder table is not used to keep the instanceKlass alive for GC during class resolution, and no
>> other logic depends on the contents of the placeholder table. Therefore rather than retaining the entries for the entire
>> span of resolve_from , entries are explicitly added and removed for each specific resolution action: LOAD_INSTANCE,
>> LOAD_SUPER and DEFINE_CLASS (used for parallel class loading).
>> 
>> Tests run:
>> Original test case from bug (linux 32 bit client)
>> regression tests on solaris/sparc 32 bit:
>>   vm.sysdict.testlist
>>   vm.quick.testlist
>>   vm.parallel_class_loading.testlist: no flags, -XX:+AllowParallelDefineClass, -XX:+UnsyncLoadClass, -XX:+PrintSystemDictionaryAtExit
>>   java.lang.instrument: jtreg
>>   jli with -Xcomp
>>   ctw nightly.testlist
>> tests in progress on solaris/sparc 32 bit:
>>   runThese -Xcomp
>>   jck vm
>> 
>> thanks,
>> Karen
>> 
>> 
>