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

[David Holmes]

Hi Goetz,

Sorry really strapped for time these days ...

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

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

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.

Only comment I'll make on the tests is that being too specific about the 
exact message makes the tests more fragile.

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