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

[Lois Foltan]

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.

Thanks,
Lois

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