(11) RFR (S) JDK-8205509: assert(_name_and_id != 0LL) failed: encountered a class loader null name and id

[Thomas Stüfe]

Hi Lois,

This looks good, thanks for fixing.

I guess that means the return value of CLD::name() and
CLD::name_and_id() may "fluctuate" under rare conditions, yes?

You could make _class_loader_klass const and initialize it in the
CLD::CLD initializer list.

I try to understand why we cannot initialize _name and _name_and_id in
the constructor too. In ClassLoaderDataGraph::add() I see we do
initialize them explicitly after adding them to the CLDG because
"because adding the Symbol for the name might safepoint." (btw I like
safepoint as a verb :-). But where, in
ClassLoaderData::initialize_name_and_klass(), could we possibly
safepoint? Sorry if the question is stupid. The symbol lives in C-heap
(resp. arena, which in the end is the same), so allocating a Symbol
should not safepoint?

If the comment in CLDG::add() is still correct, could you modify it to
"adding the Symbols for name and name_and_id"?

Thank you, Thomas

On Sat, Jun 23, 2018 at 1:39 AM, Lois Foltan <lois.foltan at oracle.com> wrote:
> On 6/22/2018 7:28 PM, coleen.phillimore at oracle.com wrote:
>
>>
>> Lois,
>> This looks good!
>>
>>
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8205509/webrev/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html
>>
>>    if (!h_class_loader.is_null()) {
>>      _class_loader = _handles.add(h_class_loader());
>>    }
>>
>> + if (!h_class_loader.is_null()) {
>> + _class_loader_klass = h_class_loader->klass();
>> + }
>> +
>>
>> Can you put the _class_loader_klass under the if statement above it?
>>
>> + } else {
>> + return _class_loader_klass->external_name();
>>
>> Can you add short comment:  // May be called in a race before _name_and_id
>> is initialized.
>>
>> Unfortunately, there isn't a good way to assert this (other than maybe a
>> flag which would serve the same purpose as a null _name_and_id), and this
>> code path is sensitive to safepoints because the link from class_loader to
>> ClassLoaderData and the addition of ClassLoaderData to the CLDG needs to
>> appear atomic.
>
> Hi Coleen,
>
> Thanks for the review, good comments.  They all have been addressed and more
> testing is in progress.  See updated webrev at:
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8205509.1/webrev/
>
> Lois
>
>
>>
>> Thanks for the speedy fix.
>> Coleen
>>
>> On 6/22/18 7:00 PM, Lois Foltan wrote:
>>>
>>> Please review the following change to address a hs-tier5 test failure.
>>> The recent change for JDK-8202605 aggravated an existing race condition
>>> within ClassLoaderDataGraph::add() where a given class loader's
>>> ClassLoaderData structure is added to the ClassLoaderDataGraph prior to full
>>> initialization of the CLD's _class_loader_klass, _name and _name_and_id
>>> fields within ClassLoaderData::initialize_name_and_klass.  To ensure that
>>> methods loader_name() and loader_name_and_id() always provide a string for
>>> the class loader name, the fix involved moving the field,
>>> _class_loader_klass, to the ClassLoaderData's constructor where if _name
>>> and/or _name_and_id fields are null, a reasonable _class_loader_klass's
>>> external_name() can be provided.
>>>
>>> open webrev http://cr.openjdk.java.net/~lfoltan/bug_jdk8205509/webrev/
>>> bug link at https://bugs.openjdk.java.net/browse/JDK-8205509
>>>
>>> Testing: hs-tier(1-2), jdk-tier(1-3) complete
>>>                hs-tier3-5 in progress
>>>                --test-repeat 50 ran for tier1 platforms of test
>>> jdk/java/lang/instrument/DaemonThread/TestDaemonThread.java
>>>
>>> Thanks,
>>> Lois
>>
>>
>