[ping] RE: RFR(M): 8199852: Print more information about class loaders in LinkageErrors.
Lois Foltan
lois.foltan at oracle.com
Thu May 3 18:24:38 UTC 2018
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