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

David Holmes david.holmes at oracle.com
Wed May 2 12:56:43 UTC 2018 

Ps.

On 2/05/2018 9:21 PM, 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!

Thanks for that.

Cheers,
David
-----

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