RFR(M): 8199852: Print more information about class loaders in LinkageErrors.
David Holmes
david.holmes at oracle.com
Tue Mar 27 01:22:27 UTC 2018
Hi Lois,
<trimming>
On 27/03/2018 5:58 AM, Lois Foltan wrote:
> On 3/25/2018 5:48 PM, David Holmes wrote:
>> 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.
Sorry Lois but I don't see that the toString() method of
StackTraceElement is an authoritive definition of how loaders should be
described in other contexts. That toString definition has to combine
module, loader, class and method, information in a compact form. But
that is not what we are dealing with here. Had I been involved in that
work I would have also argued against completely omitting loader
information in the un-named case.
> 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.
Yes but we are not dealing with module names here. And the fact it
completely ignores un-named loaders makes it inherently unsuitable for
the current context.
The issue of whether information about the parent loader is suitable for
the error message or should be left for logging, depends on how you
expect these messages to help. If the idea is for the exception message
to provide enough information for a support engineer to be able to
diagnose a problem without asking the submitter to re-run with logging
enabled, then you want to gather as much relevant information as
possible. But a line has to be drawn somewhere. I don't think listing
the loader's parent is excessive in this case, but nor do I see it as
essential.
Thanks,
David
>
> 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