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 
StackTraceElement.toString(), 
https://docs.oracle.com/javase/9/docs/api/java/lang/StackTraceElement.html. 
For example, the preference as stated for "unnamed" loaders is:

If the class loader is abuilt-in class loader 
<https://docs.oracle.com/javase/9/docs/api/java/lang/ClassLoader.html#builtinLoaders>or 
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.

Thanks,
Lois

>
>>> 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