8202605 ... was RE: [ping] RE: RFR(M): 8199852: Print more information about class loaders in LinkageErrors.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri May 4 15:14:08 UTC 2018
Hi Lois,
I had a look at the Jira issue you opened. I wanted to address the issue.
https://bugs.openjdk.java.net/browse/JDK-8202605
First I was puzzled, but then I realized major parts of it have been done
in 8201556: Disallow reading oops in ClassLoaderData if unloading
http://hg.openjdk.java.net/jdk/jdk/rev/e242740a92b8
For replacing java_lang_ClassLoader::name() by ClassLoaderData::loader_name():
I don't think this makes sense, as the first is the mere accessor for the field
specified in the Java class java.lang.Classloader, and the second (respective
SystemDictionary::loader_name()) is supposed to return some useful
information in any case, also if the name field is not set.
Nevertheless I would like to see the content of the name field
printed in SystemDictionary::loader_name() if it is set.
And probably SystemDictionary::loader_name() should be replaced
by ClassLoaderData::loader_name().
What do you think?
Best regards,
Goetz.
> -----Original Message-----
> From: Lois Foltan [mailto:lois.foltan at oracle.com]
> Sent: Donnerstag, 3. Mai 2018 20:25
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; David Holmes
> <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: [ping] RE: RFR(M): 8199852: Print more information about class
> loaders in LinkageErrors.
>
> 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