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

[Lois Foltan]

On 6/23/2018 3:03 AM, Thomas Stüfe wrote:

> Hi Lois,
>
> This looks good, thanks for fixing.
Thanks for the review Thomas!
>
> I guess that means the return value of CLD::name() and
> CLD::name_and_id() may "fluctuate" under rare conditions, yes?
Yes.

>
> You could make _class_loader_klass const and initialize it in the
> CLD::CLD initializer list.
I could except that the bootstrap class loader case has to be checked 
before setting _class_loader_klass to the oop's klass().

>
> 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?
The issue is within ClassLoaderData's constructor and I'm borrowing 
Coleen's description from a previous email about this.

It's just kind of an inconvenience and at a sketchy time.  If you look 
at the constructor ClassLoaderData::ClassLoaderData, you'd have to have 
the safepoint before lines:

   if (!h_class_loader.is_null()) {
     _class_loader = _handles.add(h_class_loader());
   }

Because the ClassLoaderData hasn't been added to the graph yet, and if 
there's some GC, this _handles[] cell will not get followed. We've had 
many problems like this in the past because we had more oops that were 
in the ClassLoaderData.  There are just some hidden dependencies on what 
the code does at this point and having the constructor do miminal things 
helps.

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

>
> 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
>>>