[jdk11u-dev] RFR: 8251945: SIGSEGV in PackageEntry::purge_qualified_exports() [v6]

[Andrew Dinn]

On Fri, 18 Jun 2021 13:57:57 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

>> I would like to fix the crash in openjdk 11u.
>> 
>> The crash is caused by racy installing new CLD in ClassLoaderDataGraph::add_to_graph().
>> 
>> The method first creates new ClassLoaderData, and in its constructor, it creates unnamed module entry and installs it in java_lang_Module oop.
>> 
>> Then add_to_graph() tries to install newly created CLD to java_lang_ClassLoader oop via CAS. If it loses race, then it deletes new CLD and returns existing one.
>> 
>> But at this point, java_lang_Module oop still points module entry that is already freed.
>> 
>> The fix I am purposing is to borrow ClassLoaderDataGraph_lock from JDK-8210155, but only uses it to prevent racing installing CLD and new CLD is still published via CAS to avoid needing additional patches.
>> 
>> Test:
>>  - [x] hotspot_runtime
>>  - [x] hotspot_gc
>>  - [x] vmTestbase_vm_gc
>
> Zhengyu Gu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Removed trailing whitespaces

This looks fine apart from the naming of the get and set methods.

src/hotspot/share/classfile/javaClasses.cpp line 4023:

> 4021: int  java_lang_ClassLoader::unnamedModule_offset = -1;
> 4022: 
> 4023: ClassLoaderData* java_lang_ClassLoader::loader_data(oop loader) {

I understand the desire to minimize changes to related files (specifically the JFR files that currently include calls to java_lang_ClassLoader::loader_data() and java_lang_ClassLoader::set_loader_data(). However, I think it risks misleading future maintainers to leave these methods with their existing names when they are actually being modified to employ acquire/release semantics.

I think it would be much better to rename them consistently with the upstream patch so they are called java_lang_ClassLoader::loader_data_acquire() and java_lang_ClassLoader::release_set_loader_data() and then work those name changes through to all client calls. That not only retains consistency with upstream it also clarifies the local use.

-------------

Changes requested by adinn (Reviewer).

PR: https://git.openjdk.java.net/jdk11u-dev/pull/22