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

Lois Foltan lois.foltan at oracle.com
Mon Mar 26 19:58:25 UTC 2018

On 3/25/2018 5:48 PM, David Holmes wrote:

> Hi Goetz,
> On 23/03/2018 9:49 PM, Lindenmaier, Goetz wrote:
>> 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.
> I have no issue providing information about loaders when they are 
> relevant - as in this case.

Hi Goetz,

I too have no issue with providing information about loaders in this 
case.  However, including information about the parent loader seems to 
me a bit verbose and may be more apropos for logging than actually 
including it within the LinkageError itself.

>> 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.)
> 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 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?
> 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"
>>> 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.
> 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> ...
> not sure how much detail about the parent is needed.

I do not support the above pattern for the descriptive text.  The 
supported pattern has already been documented within the API Note of 
For example, the preference as stated for "unnamed" loaders is:

If the class loader is abuilt-in class loader 
is not named then the first element and its following|"/"|are omitted as 
shown in "|acme at 2.1/org.acme.Lib.test(Lib.java:80)|"

Again, Klass::class_loader_module_name() already implement and adhere to 
this pattern.


>>> 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.
> Okay.
>>> 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).
> "sig" may be inaccurate but not in the way you describe. At the 
> language level the signature of a method is the name plus the 
> arguments, but not the return type. At the VM level a signature is all 
> the types:
> MethodSignature:
> [TypeParameters] ( {JavaTypeSignature} ) Result {ThrowsSignature}
> but not the  name itself. So I don't think "sig" is misleading here at 
> all.
>> 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 :)
> In LinkResolver::check_*_loader_constraints method and field 
> information is quoted the same - and in both cases to stop it running 
> into the preceding class name. The only other places methods are 
> referred to are in klassVtable.cpp where again the two occurrences 
> were quoted. I can't speak to any global policy on this as obviously 
> no such policy exists, but these related messages were consistent with 
> each other.
>>> 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?
> I think yes to both. Expand the test for missing cases (there should 
> not be missing cases!) and add the additional message checking. Add 
> this bug to the @bug in the test(s).
>>> 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).
> Just pointing out these utilities exist (custom loaders in particular 
> seem to be strewn throughout the test).
>>> 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 :(
> That's fine by me - I'm swamped with other things. ;-)
> Thanks,
> David
> -----
>> 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