RFR(M): 8199852: Print more information about class loaders in LinkageErrors.
David Holmes
david.holmes at oracle.com
Thu Apr 12 06:50:24 UTC 2018
Hi Goetz,
On 11/04/2018 2:03 AM, Lindenmaier, Goetz wrote:
> 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.
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
Thanks,
David
> 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