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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Wed May 2 11:21:07 UTC 2018 

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