[ping] RE: RFR(M): 8199852: Print more information about class loaders in LinkageErrors.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed May 2 15:19:41 UTC 2018
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.
> 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