RFR 8210422: runtime/modules/ModuleStress/ExportModuleStressTest.java - assertion failed: address not aligned: 0x00000008baadbabe

[coleen.phillimore at oracle.com]

On 9/11/18 10:00 AM, Lois Foltan wrote:
> On 9/10/2018 5:00 PM, coleen.phillimore at oracle.com wrote:
>
>>
>>
>> On 9/10/18 2:07 PM, Lois Foltan wrote:
>>> On 9/10/2018 12:06 PM, coleen.phillimore at oracle.com wrote:
>>>
>>>>
>>>>
>>>> On 9/10/18 11:55 AM, Lois Foltan wrote:
>>>>> On 9/10/2018 9:37 AM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>>> Summary: CLDG_lock caused safepoint in inconsistent state
>>>>>>
>>>>>> See bug comments for more details.
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8210422.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8210422
>>>>>>
>>>>>> Tested with mach5 hs-tier1-7.  There are good parallel class 
>>>>>> loading tests that test this code.  Unfortunately these tests 
>>>>>> aren't open yet.
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>
>>>>> Looks good.  Minor comment:
>>>>>
>>>>> share/classfile/classLoaderData.cpp:
>>>>> line #1127 - you removed the comment "because adding the Symbol 
>>>>> for _name and _name_and_id might safepoint".  Isn't that still the 
>>>>> case?  If not, then why are we initializing those fields after the 
>>>>> loader data has been added to the CLDG and not before?
>>>>
>>>> I removed the comment because SymbolTable::new_symbol() doesn't 
>>>> safepoint anymore.  Gerard made SymbolTable_lock into 
>>>> SymbolArena_lock that doesn't check for safepoint.  But I thought 
>>>> it was best to keep the initializing name code where it was, in 
>>>> case it ever safepointed again.
>>>
>>> Ok thanks for the explanation!  Can you open an RFE for this because 
>>> it would be much cleaner to move the initialization of those 2 
>>> fields to the ClassLoaderData's ctor which now sounds doable?
>>
>> Since there were two questions about the same code, I moved the call 
>> to initialize_name to the constructor of ClassLoaderData, which is a 
>> lot nicer.  Reran parallel class loading tests, mach5 tier1,2 for 
>> verification, and reran these tests that look at the name of the 
>> class loader data (thanks Lois for the list).
>>
>>  make run-test TEST=test/hotspot/jtreg/runtime/modules
>>  make run-test TEST=test/hotspot/jtreg/runtime/LoaderConstraints
>>  make run-test 
>> TEST=test/hotspot/jtreg/runtime/SharedArchiveFile/DumpSymbolAndStringTable.java
>>  make run-test 
>> TEST=test/hotspot/jtreg/runtime/logging/LoaderConstraintsTest.java
>>  make run-test 
>> TEST=jdk/java/lang/instrument/DaemonThread/TestDaemonThread.java
>>
>> The change is only in classLoaderData.cpp (sorry forgot to do 
>> incremental before editing).
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8210422.02/webrev
>
> Looks good.  I think we should open an RFE to initialize even the boot 
> loader's _name and _name_and_id fields to BOOTSTRAP_LOADER_NAME during 
> CLD construction, but that is minor and outside the scope of this change.

Thank you!  That's a good suggestion.  Since we now can initialize the 
name in the constructor, some of the changes for the problems we saw 
with accessing it beforehand can be cleaned up now.  More RFEs!
thanks,
Coleen

>
> Thanks,
> Lois
>
>>
>> Thanks!
>> Coleen
>>>
>>> Thanks,
>>> Lois
>>>
>>>>
>>>> thanks,
>>>> Coleen
>>>>
>>>>>
>>>>> Thanks,
>>>>> Lois
>>>>
>>>
>>
>