[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR 8202913: loader constraint message for fields specifies incorrect referring class
From: Harold David Seigel <harold.seigel () oracle ! com>
Date: 2018-05-31 13:08:43
Message-ID: 0d795016-3c9d-ded1-cac4-b8e59a2a5011 () oracle ! com
[Download RAW message or body]
Thanks!
Harold
On 5/30/2018 11:40 AM, Lindenmaier, Goetz wrote:
> Hi Harold,
>
>> ... AppClassLoader) for the class|abstractclass|interface test.Parent defining
> I meant you should print the proper identifier, not the text with '|' by using
> "... %s ..." , ..., k->external_kind(), ...
> As this replaces 'type' by one or two (abstract class) words I don't think
> it makes the message more wordy, just more precise. See also patch below
> (just edited, not tested). Feel free to ignore my proposal, and else I don't
> need a new webrev in case you adapt this.
>
> The rest looks good. Great it's all with '.' and you moved the test!
>
> Best regards,
> Goetz.
>
> diff -r 6b4a85ad6bea src/hotspot/share/interpreter/linkResolver.cpp
> --- a/src/hotspot/share/interpreter/linkResolver.cpp Wed May 30 17:32:28 2018 +0200
> +++ b/src/hotspot/share/interpreter/linkResolver.cpp Wed May 30 17:37:04 2018 +0200
> @@ -690,19 +690,20 @@
> const char* msg = "loader constraint violation: when resolving field"
> " \"%s\" of type %s, the class loader %s of the current class, "
> "%s, and the class loader %s for the field's defining "
> - "type, %s, have different Class objects for type %s";
> + "%s, %s, have different Class objects for type %s";
> const char* field_name = field->as_C_string();
> const char* loader1_name = java_lang_ClassLoader::describe_external(ref_loader());
> const char* sel = sel_klass->external_name();
> + const char* sel_type = sel_klass->external_kind();
> const char* loader2_name = java_lang_ClassLoader::describe_external(sel_loader());
> const char* failed_type_name = failed_type_symbol->as_klass_external_name();
> const char* curr_klass_name = current_klass->external_name();
> size_t buflen = strlen(msg) + strlen(field_name) + 2 * strlen(failed_type_name) +
> strlen(loader1_name) + strlen(curr_klass_name) +
> - strlen(loader2_name) + strlen(sel) + 1;
> + strlen(loader2_name) + strlen(sel) + strlen(sel_type) + 1;
> char* buf = NEW_RESOURCE_ARRAY_IN_THREAD(THREAD, char, buflen);
> jio_snprintf(buf, buflen, msg, field_name, failed_type_name, loader1_name,
> - curr_klass_name, loader2_name, sel, failed_type_name);
> + curr_klass_name, loader2_name, sel_type, sel, failed_type_name);
> THROW_MSG(vmSymbols::java_lang_LinkageError(), buf);
> }
> }
>
>
>
>> -----Original Message-----
>> From: Harold David Seigel [mailto:harold.seigel@oracle.com]
>> Sent: Mittwoch, 30. Mai 2018 15:07
>> To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; hotspot-runtime-
>> dev@openjdk.java.net
>> Subject: Re: RFR 8202913: loader constraint message for fields specifies
>> incorrect referring class
>>
>> Hi Goetz,
>>
>> Thanks for reviewing this change!
>>
>>
>> Please review this updated webrev for this bug:
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8202913.2/webrev/index.h
>> tml
>>
>> The new exception message for the included test looks like this:
>>
>> loader constraint violation: when resolving field "_field1" of type
>> pkg.Foo, the class loader "<unnamed>" (instance of
>> pkg.ClassLoaderForChildGrandFoo, child of "app"
>> jdk.internal.loader.ClassLoaders$AppClassLoader) of the current class,
>> pkg.Child, and the class loader "<unnamed>" (instance of
>> pkg.ClassLoaderForParentFoo, child of "app"
>> jdk.internal.loader.ClassLoaders$AppClassLoader) for the field's defining
>> type, pkg.Parent, have different Class objects for type pkg.Foo
>>
>>
>> Please also see comments in-line.
>>
>>
>>
>> On 5/25/2018 10:23 AM, Lindenmaier, Goetz wrote:
>>
>>
>> Hi,
>>
>>
>> I think this would read better if you replace 'type' by what is
>> returned by
>> Klass::external_kind().
>>
>> correcting myself, you can just say 'class' ... interfaces don't define
>> fields ...
>>
>> Karen pointed out that interfaces can have public static fields. So, I'll keep it
>> as 'type', not 'class'.
>>
>>
>>
>>
>> Best regards,
>> Goetz.
>>
>>
>>
>> -----Original Message-----
>> From: Lindenmaier, Goetz
>> Sent: Freitag, 25. Mai 2018 16:18
>> To: 'Harold David Seigel' <harold.seigel@oracle.com>
>> <mailto:harold.seigel@oracle.com> ; hotspot-runtime-
>> dev@openjdk.java.net <mailto:dev@openjdk.java.net>
>> Subject: RE: RFR 8202913: loader constraint message for fields
>> specifies
>> incorrect referring class
>>
>> Hi Harold,
>>
>> Thanks for adding this further improvement over my change.
>>
>>
>> ... AppClassLoader) for the field's
>> defining type, Parent, have different Class objects for
>> type Foo
>>
>> I think this would read better if you replace 'type' by what is
>> returned by
>> Klass::external_kind().
>>
>> Actually I would prefer to read
>> ... AppClassLoader) for the class|abstractclass|interface
>> test.Parent defining
>> this field,
>> have different Class objects for type Foo
>>
>> The message is already too wordy. I'd prefer to not add this additional text
>> because it is not related to the reason for the exception.
>>
>>
>>
>>
>> Could you please add the test files into some package so that
>> you
>> can assure class names are printed as test.Parent and not
>> test/Parent?
>> There is external_name() to get the proper names from
>> classes, I don't
>> know for symbols.
>> Related tests are in runtime/LoaderConstraint. Please move
>> your test
>> there.
>>
>> Done.
>>
>> Thanks, Harold
>>
>>
>>
>>
>> Best regards,
>> Goetz.
>>
>>
>>
>>
>>
>> -----Original Message-----
>> From: hotspot-runtime-dev [mailto:hotspot-runtime-
>> dev-
>> bounces@openjdk.java.net
>> <mailto:bounces@openjdk.java.net> ] On Behalf Of Harold David Seigel
>> Sent: Freitag, 25. Mai 2018 15:29
>> To: hotspot-runtime-dev@openjdk.java.net
>> <mailto:hotspot-runtime-dev@openjdk.java.net>
>> Subject: RFR 8202913: loader constraint message for
>> fields specifies
>>
>> incorrect
>>
>> referring class
>>
>> Hi,
>>
>> Please review this change to correct and simplify the
>> error message
>> displayed when a loader constraint check fails when
>> trying to access a
>> field.
>>
>> The old message (for this test case):
>>
>> loader constraint violation: when resolving field
>> "_field1" the
>> class loader "<unnamed>" (instance of
>> ClassLoaderForChildGrandFoo,
>> child of "app"
>> jdk.internal.loader.ClassLoaders$AppClassLoader) of
>> the referring class, Parent, and the class loader
>> "<unnamed>"
>> (instance of ClassLoaderForParentFoo, child of
>> "app"
>> jdk.internal.loader.ClassLoaders$AppClassLoader)
>> for the field's
>> resolved type, Foo, have different Class objects for
>> that type
>>
>> The new message:
>>
>> loader constraint violation: when resolving field
>> "_field1" of type
>> Foo, the class loader "<unnamed>" (instance of
>> ClassLoaderForChildGrandFoo, child of "app"
>> jdk.internal.loader.ClassLoaders$AppClassLoader)
>> of the current
>> class, Child, and the class loader "<unnamed>"
>> (instance of
>> ClassLoaderForParentFoo, child of "app"
>> jdk.internal.loader.ClassLoaders$AppClassLoader)
>> for the field's
>> defining type, Parent, have different Class objects
>> for type Foo
>>
>> Open Webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_8202913/webrev/
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-
>> 8202913
>>
>> This fix was tested with Mach5 tiers 1 and 2 tests and
>> builds on
>> Linux-X64, Windows, Solaris Sparc, and Mac OS X, with
>> tiers 3-5 tests on
>> Linux-x64, and with JCK-11 Lang and VM tests.
>>
>> Thanks, Harold
>>
>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic