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

[David Holmes]

Hi Goetz,

On 21/03/2018 11:55 PM, David Holmes wrote:
> On 21/03/2018 11:34 PM, Lindenmaier, Goetz wrote:
>> Hi David,
>>
>> 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:

"loader <name> (instance of <class>) previously initiated loading for a 
different type with name "T" with defining loader <name2> (instance of 
<class2>)"

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.

---

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.

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

---

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.

---

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

There are various existing utilities in the test library that might make 
things easier e.g. ByteCodeLoader.

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

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


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