RFR(M): 8199852: Print more information about class loaders in LinkageErrors.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Apr 10 16:03:11 UTC 2018
Hi,
finally I prepared a new webrev.
This contains what I promised to change in the mail cited below.
But mostly it contains changes moving the tests to the LoaderConstraints
directory.
This gave me the opportunity to add two more tests for my
messages (vtable, itable) and to add tests of two more exceptions.
I added a package to most test classes, to check that classnames are
printed with '.' -- and found that I had missed one output in the
vtable/itable messages. I also use ClassFileInstaller now.
Incremental webrev of the changes to hotspot:
http://cr.openjdk.java.net/~goetz/wr18/8199852-exMsg_Linkage/02-incremental/
The full webrev with the adapted tests.
http://cr.openjdk.java.net/~goetz/wr18/8199852-exMsg_Linkage/02/
I hope the basic layout of the tests is good now. I'm happy to adapt
the messages based on this, further. The new change runs through our
testing tonight.
Best regards,
Goetz.
> -----Original Message-----
> From: Lindenmaier, Goetz
> Sent: Freitag, 23. März 2018 12:50
> 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,
>
> 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