RFR(M): 8199852: Print more information about class loaders in LinkageErrors.
David Holmes
david.holmes at oracle.com
Sun Mar 25 21:48:38 UTC 2018
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.
> 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.
>> 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