RFR(M): 8199852: Print more information about class loaders in LinkageErrors.

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Fri Mar 23 11:49:37 UTC 2018 

Hi David, 

Thanks for your detailed comments.

A general remark why we would like to have the loader information
in these messages, even if the loaders are not the cause for the
exception.
SAP has a lot of huge Java systems, built by various teams. Our 
support usually can not access the systems causing errors to 
debug etc.
Even if the class loader is not the cause for the exception, the 
information about it helps to track down where in the system
the problem occurred. And often it's the parent class loader
that helps.
Obviously, my test programs that report "app" and "platform"
class loaders are too simple to show this use case of the information.

Maybe I should suppress printing verbose information about these
well known loaders.  (There is is_builtin_class_loader_data() to 
identify such.)

> >> I put that description into the bug:
> >> https://bugs.openjdk.java.net/browse/JDK-8199852
> >> I thought it should be better archived than in the mail thread.
> >>
> >> The change you mention below is intentional. There are two
> >> different loaders involved, and so far only one of them was printed.
> >> I tried to put "different type" info more detailed wording, too
> >> (different class with the same name).
> 
> The point is the current loader is already marked as an initiating
> loader for a type (not class but type) T, and is now trying to be an
> initiating loader for a different type that is also called T. What you
> are doing is providing information on the defining loader for the
> existing T. If you think that's useful additional information then I
> suggest phrasing it more succinctly (exception messages should never
> consist of multiple sentences!) e.g:
Yes, I think it is useful to know about both classes involved in the clash.

> "loader <name> (instance of <class>) previously initiated loading for a
> different type with name "T" with defining loader <name2> (instance of
> <class2>)"
I'm not clear where you distinguish type and class.
Is it that a class can result in two types if it is loaded by two diffrerent loaders?
And  a type is that for which:
objects a, b have the same type if
a instanceof b.class && b instanceof a.class holds?
 
> Looking through the rest ...
> 
> src/hotspot/share/classfile/javaClasses.cpp
> 
> If the loader does not have a name I think you still need to say
> "instance of <classname>" to be clear "classname" is not a name.
OK, I'll change that. I thought using "" just for names would help here.
Unfortunately printing 'instance of' in describe_extenal makes using
it less flexible.

> src/hotspot/share/classfile/systemDictionary.cpp
> !         guarantee(check->class_loader() == class_loader(), "Per
> construction. Else report the other loader."); 
> Do we really need a new guarantee? At most an assert, but I'm not sure
> what you're trying to guard against.
Just a left over ... removed.

> 3111 const char* SystemDictionary::loader_name(const oop loader) {
> I don't think you should be hijacking loader_name to become loader
> description! And if you did do this then you wouldn't need to replace
> uses of loader_name with describe_external (as in klassVtable.cpp.
> linkResolver.cpp).
 - I think linkResolver is a bad place to put a method like this. I did not
   want to change all call sites of this, though.
- I think the information printed now would be useful In tracing, too.
But I reverted it.  The point of this change are the LinkageError messages.
 
> src/hotspot/share/oops/klassVtable.cpp
> 
> !                 "overriding method %s the class loader %s of the "
> 
> Why did you get rid of the quoting around the method sig? The quoting
> delimits the sig more clearly and also allows you to see unexpected
> occurrences like an empty sig more clearly.
The variable 'sig' is misleading. It contains method and signature. How should the 
name of a method be emtpy? And the sig is at least ()V, isn’t it?
(I renamed sig to method).

There is no consistency with the "" in the messages.  I wanted to improve
on that. In my last change I didn't quote the method's either. 
I think the only thing that should be quoted are user defined strings.
If methods are quoted, fields and classes should be quoted, too. In all 
messages.  If this is consent, I'll do a follow up change fixing it in all
the messages I touched :)
 
> Testing comments:
> 
> I'd rather see the tests as additions to and/or modifications to the
> existing loaderConstraint tests than "exceptionMsgs" tests. In that
> regard the existing loaderConstraint tests should already cover many of
> the different forms of LinkageErrors expected. 
Thanks for pointing me to these. 
They exercise the two messages in klassVTable for which I didn't
have a test yet. They do not exercise the other cases.
Should I move my test there?  Can I add the message check into 
the existing tests?

> All you are doing here is
> adding some detail to the messages - the testing should not need to be
> so elaborate and complex. Plus you may be able to reuse utilities like
> the PreemptingClassLoader.
> 
> You should be able to use ClassFileInstaller to move the .class files
> around rather than a shell script. We're trying to get rid of shell
> scripts in the tests.
I'll look into this. I didn't like the script either.
 
> There are various existing utilities in the test library that might make
> things easier e.g. ByteCodeLoader.
This loader can load only one class. The name must be given in
the constructor. It does not set the name field.  So I would have to 
extend it to use it (take a list of strings for classes, constructor with names).

> New files should only have a single, current, copyright year.
Fixed.

> I now see you copied a lot of stuff from runtime/6626217 but I don't
> think it is a particularly great example to copy. 
This is the only test that excercises this code I found. After successless 
manual search I ran the nightbuild with a guarantee() which runs much more
tests, including SAP tests.  I found some jck tests, too, but I decided 
to modify this one as it's open source.

> Again the
> loaderConstraint tests should be a better starting place. In some cases
> the comments are not relevant eg LE2_D_ambgs.jasm:
> 
>   32 // public class LE2_D_ambgs extends bug_21227 {
>   35 //         _ref_to_be_p0wned = LE2_C._p0wnee;
Removed.
 
I'll try to come up with another webrev today.  But I'm out-of-office
the next two weeks, so my work on the issue might be a bit slower :(

Best regards,
  Goetz.


> 
> Thanks,
> David
> -----
> 
> 
> > I need to look into this in more detail tomorrow (sorry nearly midnight
> > here). I thought that message was for a case involving only one loader.
> > (I added these detailed messages way back when.)
> >
> > David
> >
> >> Unfortunately I could not come up with a test for the messages
> >> in klassVtable.cpp, but there I just removed the "instance of",
> >> that appears in the printed message if the classloader has a name.
> >>
> >> Any advice when there should be "" in the message? I normalized
> >> it so that only the user-definable names are quoted. I removed
> >> the "" from around classnames, methods etc.
> >>
> >> Best regards,
> >>    Goetz.
> >>
> >>
> >>> -----Original Message-----
> >>> From: David Holmes [mailto:david.holmes at oracle.com]
> >>> Sent: Mittwoch, 21. März 2018 14:12
> >>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
> runtime-
> >>> dev at openjdk.java.net
> >>> Subject: Re: RFR(M): 8199852: Print more information about class
> >>> loaders in
> >>> LinkageErrors.
> >>>
> >>> Hi Goetz,
> >>>
> >>> Can you provide a detailed comparison of all the before and after
> >>> messages please. It seems to me you are rewriting some of the
> messages
> >>> in a way that changes the meaning.
> >>>
> >>> src/hotspot/share/classfile/systemDictionary.cpp
> >>>
> >>> Before:
> >>>
> >>> !         linkage_error1 = "loader constraint violation: loader
> >>> (instance of ";
> >>> !         linkage_error2 = ") previously initiated loading for a
> >>> different type with name \"";
> >>>
> >>> after:
> >>>
> >>> !         linkage_error1 = "loader constraint violation: loader ";
> >>> !         linkage_error2 = " wants to load class ";
> >>> !         linkage_error3 = ". A different class with the same name was
> >>> previously loaded by ";
> >>>
> >>> this does not describe the same situation!
> >>>
> >>> Thanks,
> >>> David
> >>>
> >>> On 21/03/2018 9:55 PM, Lindenmaier, Goetz wrote:
> >>>> Hi,
> >>>>
> >>>> ClassLoaders have a field containing a name since Java 9.
> >>>>
> >>>> This change improves some of the messages of LinkageErrors to
> >>>> print the name of the loader involved, as well as the parent of the
> >>>> loader.
> >>>> This simplifies tracking down the component causing an error in large
> >>>> Java systems.
> >>>>
> >>>> Please review:
> >>>> http://cr.openjdk.java.net/~goetz/wr18/8199852-
> >>> exMsg_Linkage/01/index.html
> >>>>
> >>>> I ran this through all our tests (jck, jtreg, spec etc).  I will run
> >>>> it through
> >>> submit-hs
> >>>> (or jdk/submit) once it's reviewed.
> >>>>
> >>>> Best regards,
> >>>>     Goetz.
> >>>>
> >>>>
> >>>>
> >>>>

More information about the hotspot-runtime-dev mailing list