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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Fri May 4 19:04:15 UTC 2018 


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

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