8202605 ... was RE: [ping] RE: RFR(M): 8199852: Print more information about class loaders in LinkageErrors.

Lois Foltan lois.foltan at oracle.com
Fri May 4 20:35:59 UTC 2018 

On 5/4/2018 3:04 PM, coleen.phillimore at oracle.com wrote:

>
>
> On 5/4/18 11:14 AM, Lindenmaier, Goetz wrote:
>> Hi Lois,
>>
>> I had a look at the Jira issue you opened. I wanted to address the 
>> issue.
>> https://bugs.openjdk.java.net/browse/JDK-8202605
>> First I was puzzled, but then I realized major parts of it have been 
>> done
>> in 8201556: Disallow reading oops in ClassLoaderData if unloading
>> http://hg.openjdk.java.net/jdk/jdk/rev/e242740a92b8
>
> Yes, I have to admit that I didn't carry my change far enough as I was 
> focused on not touching _class_loader oop after it's been unloading.
>>
>> For replacing java_lang_ClassLoader::name() by 
>> ClassLoaderData::loader_name():
>> I don't think this makes sense, as the first is the mere accessor for 
>> the field
>> specified in the Java class java.lang.Classloader, and the second 
>> (respective
>> SystemDictionary::loader_name()) is supposed to return some useful
>> information in any case, also if the name field is not set.
>
> ClassLoaderData::loader_name() gets the java_lang_ClassLoader::name() 
> if set.  It's stored in the _class_loader_name field.  The 
> !is_unloading() case should just be removed, I think.
I agree and did add this as an additional comment to make sure that 
!is_unloading case is removed as part of the RFE.

>>
>> Nevertheless I would like to see the content of the name field
>> printed in SystemDictionary::loader_name() if it is set.
>> And probably SystemDictionary::loader_name() should be replaced
>> by ClassLoaderData::loader_name().
>
> Yes, I think that's the intent.  The code has gone back and forth on 
> this, but now ClassLoaderData::loader_name() has the best info.
I agree with this as well, the intent of the RFE is to prefer 
ClassLoaderData::loader_name() over SystemDictionary::loader_name().
Thanks,
Lois

>
> One word about your error message change.  For CLD::loader_name() I 
> think printing the parent loader and "<unnamed>" is too wordy for 
> logging and I don't want to see it printed.  I look at this output a lot!
>
> Thanks,
> Coleen
>> What do you think?
>>
>> Best regards,
>>    Goetz.
>>
>>
>>
>>
>>> -----Original Message-----
>>> From: Lois Foltan [mailto:lois.foltan at oracle.com]
>>> Sent: Donnerstag, 3. Mai 2018 20:25
>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; David Holmes
>>> <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net
>>> Subject: Re: [ping] RE: RFR(M): 8199852: Print more information 
>>> about class
>>> loaders in LinkageErrors.
>>>
>>> On 5/2/2018 11:19 AM, Lindenmaier, Goetz wrote:
>>>
>>>> Hi Lois,
>>>>
>>>> Thanks for your approval of my change!
>>>>
>>>>> Hi Goetz,
>>>>>
>>>>> I'm ok with going forward with this change, you have my review.
>>> However,
>>>>> I would like to record some concerns I do have about this change 
>>>>> which I
>>>>> hope RFEs will be created for to clean up.
>>>>>
>>>>> 1. the change introduces another duplicate way to obtain the class
>>>>> loader's name via java_lang_ClassLoader::describe_external, where
>>>>> Klass::class_loader_and_module_name() exists today and could be
>>> modified
>>>> As stated before, I think it would be quite complicated to design one
>>> method
>>>> that delivers a name/string as complex as that of these methods to 
>>>> fit all
>>> purposes.
>>>> I appreciate though to find a common way to put the messages, common
>>>> wording etc.. I think class_loader_and_module_name() could use
>>>> describe_external() for the class_loader name.
>>>>
>>>>> 2. the method describe_external() obtains the class loader's name 
>>>>> in a
>>>>> different manner than SystemDictionary::loader_name(). How to obtain
>>>>> the class loader's name should be standardized within the VM.
>>>> In some intermediate design, I had called describe_external within
>>> loader_name.
>>>> As I understand, loader_name does not report the name of the loader at
>>> all,
>>>> only the class name.  This really should be improved.
>>> Hi Goetz,
>>>
>>> I've entered an RFE to standardize on the use of
>>> ClassLoaderData::loader_name() as the preferred way within the VM to
>>> obtain a class loader's name.  See
>>> https://bugs.openjdk.java.net/browse/JDK-8202605.  The class loader's
>>> name is set within ClassLoaderData::initialize_name_and_klass().    The
>>> aim is to remove SystemDictionary::loader_name() and all independent
>>> calls to java_lang_ClassLoader::name() in favor of this method.
>>>
>>> Thanks,
>>> Lois
>>>
>>>>> 3. previously I expressed concerns about adding the parent loader's
>>>>> information into the LinkageError message since I think this makes 
>>>>> the
>>>>> message wordy and to me this seems like information that is more
>>>>> appropriate for logging.
>>>> I think error messages should contain as much information as possible
>>>> to track down the error.
>>>> Thanks for approving my change anyways.
>>>>
>>>> Best regards,
>>>>     Goetz.
>>>>
>>>>> Thanks,
>>>>> Lois
>>>>>
>>>>> On 5/2/2018 7:21 AM, Lindenmaier, Goetz wrote:
>>>>>> Hi David,
>>>>>>
>>>>>>> Sorry really strapped for time these days ...
>>>>>> This is ok, but I will keep pinging ... ��
>>>>>> A great, general thanks to you, as you really spend a lot of 
>>>>>> effort on
>>>>>> reviewing changes by SAP.  That helps us a lot and we appreciate
>>>>>> your efforts!
>>>>>>
>>>>>>> src/hotspot/share/classfile/javaClasses.cpp
>>>>>>> src/hotspot/share/classfile/javaClasses.hpp
>>>>>>> The description of describe_external in the hpp doesn't match the
>>>>>>> implementation in the cpp: parent: x versus child of x
>>>>>> I updated the comment:
>>>>>> -  // Prints "<name>" (instance of <classname>, parent: "<name>"
>>>>> <classname>)
>>>>>> -  // or     <classname> (parent: "<name>" <classname>).
>>>>>> +  // Prints "<name>" (instance of <classname>, child of "<name>"
>>>>> <classname>).
>>>>>> +  // If a classloader has no name, it prints <unnamed> instead. The
>>> output
>>>>>> +  // for well known loaders (system/platform) is abbreviated.
>>>>>>
>>>>>>> src/hotspot/share/classfile/systemDictionary.cpp
>>>>>>>
>>>>>>> 2208         throwException = true;
>>>>>>> 2209         ss.print("loader constraint violation: loader %s",
>>>>>>> 2210
>>>>>>> java_lang_ClassLoader::describe_external(class_loader()));
>>>>>>> 2211         ss.print(" wants to load %s %s.",
>>>>>>> 2212                  k->external_kind(), k->external_name());
>>>>>>> 2213         Klass *existing_klass =
>>>>>>> constraints()->find_constrained_klass(name, class_loader);
>>>>>>> 2214         if (existing_klass->class_loader() != 
>>>>>>> class_loader()) {
>>>>>>> 2215           ss.print(" A different %s with the same name was
>>>>>>> previously loaded by %s.",
>>>>>>> 2216 existing_klass->external_kind(),
>>>>>>> 2217
>>>>>>> java_lang_ClassLoader::describe_external(existing_klass-
>>>>>> class_loader()));
>>>>>>> 2218         }
>>>>>>>
>>>>>>> I still find this way too verbose. I won't oppose it but I don't 
>>>>>>> like
>>>>>>> it. I know you're trying to solve your remote support problem, 
>>>>>>> but I
>>>>>>> worry about the newbie trying to learn and experiment and being
>>> totally
>>>>>>> bamboozled by the exception message. (I thought the old ones were
>>> bad
>>>>>>> enough - but necessary in their level of preciseness.)
>>>>>> I understand. Thanks for not opposing.
>>>>>>
>>>>>>
>>>>>>> 3125 // Caller needs ResourceMark.
>>>>>>>
>>>>>>> Nit: Why change this line but not line 3131? In general single-line
>>>>>>> comments don't need to be written as grammatically correct 
>>>>>>> sentences
>>>>>>> with full punctuation. Only if it is a multi-sentence comment 
>>>>>>> should you
>>>>>>> need to do that.
>>>>>> Reverted, leftover from my other edits in that code that were 
>>>>>> removed.
>>>>>>
>>>>>>> Only comment I'll make on the tests is that being too specific 
>>>>>>> about the
>>>>>>> exact message makes the tests more fragile.
>>>>>> Yes, but there are so many details in the message I would like to
>>>>>> assure (like the proper name of classes with '.') that I think 
>>>>>> this is
>>>>>> necessary.
>>>>>>
>>>>>> I'll push the change through the jdk-submit forest. (It's through 
>>>>>> all
>>>>>> our tests, anyways).  After pushing, I will also update the bug to
>>>>>> list the final exception messages.
>>>>>> Can I consider it reviewed by you, then?
>>>>>>
>>>>>> Best regards,
>>>>>>      Goetz.
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>
>>>>>>> On 26/04/2018 1:30 AM, Lindenmaier, Goetz wrote:
>>>>>>>> Hi Lois, David, George or others,
>>>>>>>>
>>>>>>>> I would like to finish this one up, could I please get some
>>>>>>>> (hopefully final) reviews?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>       Goetz.
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: Lindenmaier, Goetz
>>>>>>>>> Sent: Mittwoch, 18. April 2018 09:55
>>>>>>>>> To: 'David Holmes' <david.holmes at oracle.com>; 'hotspot-runtime-
>>>>>>>>> dev at openjdk.java.net' <hotspot-runtime-dev at openjdk.java.net>
>>>>>>>>> Subject: RE: RFR(M): 8199852: Print more information about class
>>>>> loaders
>>>>>>> in
>>>>>>>>> LinkageErrors.
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I rebased the webrev to the jdk repo:
>>>>>>>>> http://cr.openjdk.java.net/~goetz/wr18/8199852-
>>> exMsg_Linkage/03-
>>>>> jdk/
>>>>>>>>> Could I please get the final go?
>>>>>>>>>
>>>>>>>>> Thanks and best regards,
>>>>>>>>>       Goetz.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> -----Original Message-----
>>>>>>>>>> From: Lindenmaier, Goetz
>>>>>>>>>> Sent: Freitag, 13. April 2018 10:10
>>>>>>>>>> To: 'David Holmes' <david.holmes at oracle.com>; hotspot-runtime-
>>>>>>>>>> dev at openjdk.java.net
>>>>>>>>>> Subject: RE: RFR(M): 8199852: Print more information about class
>>>>> loaders
>>>>>>> in
>>>>>>>>>> LinkageErrors.
>>>>>>>>>>
>>>>>>>>>> Hi David,
>>>>>>>>>>
>>>>>>>>>> I fixed what you mentioned here. I also found an older mail
>>>>>>>>>> of yours with some comments I implemented now. I copied
>>>>>>>>>> that mail's content here to have only one mail ... find 
>>>>>>>>>> references
>>>>>>>>>> to some incremental webrevs in my replies below.
>>>>>>>>>>
>>>>>>>>>> Further I moved the test classes of test differentLE/ into a 
>>>>>>>>>> package.
>>>>>>>>>>
>>>>>>>>>> Comprehensive new webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~goetz/wr18/8199852-
>>> exMsg_Linkage/03
>>>>>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: David Holmes <david.holmes at oracle.com>
>>>>>>>>>>> Sent: Thursday, April 12, 2018 8:50 AM
>>>>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
>>>>>>>>> runtime-
>>>>>>>>>>> dev at openjdk.java.net
>>>>>>>>>> ...
>>>>>>>>>>> This looks reasonable to me. Two minor comments:
>>>>>>>>>>> - updated tests need updated copyright notice
>>>>>>>>>>> - I think you can avoid duplicating test1() to test2() by 
>>>>>>>>>>> passing in
>>> the
>>>>>>>>>>>        loader name and the expected message as parameters
>>>>>>>>>> Fixed both. Incremental webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~goetz/wr18/8199852-
>>> exMsg_Linkage/03-
>>>>>>>>>> incremental2/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: David Holmes <david.holmes at oracle.com>
>>>>>>>>>>> Sent: Sunday, March 25, 2018 11:49 PM
>>>>>>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
>>>>>>>>> runtime-
>>>>>>>>>>> dev at openjdk.java.net
>>>>>>>>>> ...
>>>>>>>>>>>> Maybe I should suppress printing verbose information about
>>> these
>>>>>>>>>>>> well known loaders. (There is 
>>>>>>>>>>>> is_builtin_class_loader_data() to
>>>>>>>>>>>> identify such.)
>>>>>>>>>>> Yes it might be good to simplify things for the predefined 
>>>>>>>>>>> loaders.
>>>>>>>>>>> Though I'd use:
>>>>>>>>>>> SystemDictionary::is_platform_class_loader
>>>>>>>>>>> SystemDictionary::is_system_class_loader
>>>>>>>>>>> for that check.
>>>>>>>>>> I implemented omitting the parent for these loaders.
>>>>>>>>>>
>>>>>>>>>>> Maybe it makes sense to have a set pattern for this descriptive
>>> text,
>>>>>>>>>>> and use "unnamed" if the loader has no name:
>>>>>>>>>>>
>>>>>>>>>>> loader <name>, instanceof <class>, child of loader <name> ...
>>>>>>>>>> I implemented printing "<unnamed>" for empty names.  I would
>>> like
>>>>>>>>>> To keep the format with brackets, because the enclosing text 
>>>>>>>>>> might
>>>>> use
>>>>>>>>>> commas, too, and then the sentence structure might get 
>>>>>>>>>> confusing.
>>>>>>>>>>
>>>>>>>>>> See this incremental webrev for these two changes:
>>>>>>>>>> http://cr.openjdk.java.net/~goetz/wr18/8199852-
>>> exMsg_Linkage/03-
>>>>>>>>>> incremental1a/
>>>>>>>>>>
>>>>>>>>>>> Add this bug to the @bug in the test(s).
>>>>>>>>>> Done.
>>>>>>>>>>
>>>>>>>>>>>> I'm not clear where you distinguish type and class.
>>>>>>>>>>> Using "class" is inaccurate as it can be a class, interface 
>>>>>>>>>>> or array
>>> type.
>>>>>>>>>>> In the above the entity is a "type" as we are referring to a 
>>>>>>>>>>> <type-
>>>>> name,
>>>>>>>>>>> classloader> pair. But you can also just read it as "class 
>>>>>>>>>>> or interface
>>>>>>>>>>> or array type"
>>>>>>>>>> So why not print what it really is? I had added 
>>>>>>>>>> external_kind() for
>>> this
>>>>> in
>>>>>>>>>> My previous exception message change.
>>>>>>>>>> Unfortunately only in SystemDictionary::check_constraints() 
>>>>>>>>>> this is
>>>>> easily
>>>>>>>>>> possible, and it improves the message I think.
>>>>>>>>>>
>>>>>>>>>> Incremental webrev for these two changes:
>>>>>>>>>> http://cr.openjdk.java.net/~goetz/wr18/8199852-
>>> exMsg_Linkage/03-
>>>>>>>>>> incremental1b/
>>>>>>>>>>
>>>>>>>>>> I also implemented a test for the call to external_kind(), but I
>>> missed
>>>>> to
>>>>>>> put
>>>>>>>>>> that into
>>>>>>>>>> the incremental webrev.
>>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>>       Goetz.
>

More information about the hotspot-runtime-dev mailing list