RFR 8186092: Unnecessary loader constraints produced when there are multiple defaults

harold seigel harold.seigel at oracle.com
Tue Sep 26 19:13:31 UTC 2017


Hi David,

Thanks for looking at this change!  Please see updated webrev at:

    http://cr.openjdk.java.net/~hseigel/bug_8186092.2/webrev/index.html

and also see comments embedded below.

Thanks, Harold


On 9/26/2017 3:30 AM, David Holmes wrote:
> Hi Harold,
>
> This looks okay to me. A few comments below but only one real query.
>
> On 26/09/2017 1:21 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this JDK-10 change to fix JDK-8186092.  The change 
>> prevents the checking of loader constraints during vtable and itable 
>> creation if the selected method is an overpass method. Overpass 
>> methods are created by the JVM to throw exceptions and so should not 
>> be subjected to loader constraint checking.
>
> Okay.
>
>> Additionally, this change improves the LinkageError exception error 
>> text when a loader constraint violation occurs during vtable and 
>> itable creation.
>
> Hmmm :) I think I put those in initially. Not sure I 100% agree with 
> the changed terminology, but I'll defer to you as the current expert 
> in this area. :)
I'm hoping better experts also review the changed messages.
>
>> The fix includes four new tests, one test each to check that loader 
>> constraint checking is not done for overpass methods during vtable 
>> and itable creation, and one test each to test the new vtable and 
>> itable loader constraint error messages.
>
> *.jasm: can you add a comment indicating why these are jasm files as 
> it is not obvious to me what is special about them.
Thanks for pointing this out.  I converted the two Task.jasm files to 
Task.java file and added a comment to the remaining .jasm file, C.jasm.
>
> */Test.java:
>  - You can place multiple files on one @compile tag (and still list 
> one file per line).
> - you don't need to specify java.lang in the name of the exception 
> classes
Done.
>
>> Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8186092/webrev/
>
> The real query:
>
> 1201     if (target == NULL || !target->is_public() || 
> target->is_abstract() || target->is_overpass()) {
> 1202       // Entry does not resolve. Leave it empty for 
> AbstractMethodError.
> 1203       if (!(target == NULL) && !target->is_public()) {
> 1204         // Stuff an IllegalAccessError throwing method in there 
> instead.
> 1205         itableOffsetEntry::method_entry(_klass, 
> method_table_offset)[m->itable_index()].
> 1206 initialize(Universe::throw_illegal_access_error());
> 1207       }
>
> Not clear why you added the overpass check here? If it is non-public 
> then you're replacing it with an IllegalAccessError instead of 
> whatever the Overpass was going to throw. ??
Currently, all overpass methods are public methods. So, they would not 
get replaced with IllegalAccessError.  However, in case non-public 
overpass methods exist in the future, I added "&& 
!target->is_overpass()" to line 1203.

Alternatively, I considered adding an "assert(!target->is_overpass() || 
target->is_public(), "Non-public overpass method");" between lines 1201 
and 1202 but didn't think that this code should be concerned about 
whether or not overpass methods are public.  I also thought about adding 
"&& !target->is_overpass()" to line 1211 but thought it better that all 
checks on 'target', that prevent loader constraints checking, be done at 
the same place.
>
> Thanks,
> David
> -----
>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8186092
>>
>> The change was tested with the JCK Lang and VM tests, the JTreg 
>> hotspot, java/io, java/lang, java/util, and other tests, the 
>> co-located NSK tests, JPRT, and with RBT tier2 - tier5 tests.
>>
>> Thanks, Harold
>>



More information about the hotspot-runtime-dev mailing list