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

David Holmes david.holmes at oracle.com
Wed Sep 27 21:42:58 UTC 2017


On 27/09/2017 11:38 PM, harold seigel wrote:
> Hi David,
> 
> Please review this updated webrev at:
> 
>     http://cr.openjdk.java.net/~hseigel/bug_8186092.3/webrev/index.html
> 
> The only change from the previous webrev involves adding the assert() at 
> lines 1202 and 1203 of klassVtable.cpp.

Looks good.

Thanks,
David

> Thanks, Harold
> 
> On 9/26/2017 8:25 PM, David Holmes wrote:
>> Hi Harold,
>>
>> On 27/09/2017 5:13 AM, harold seigel wrote:
>>> 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
>>
>> Test changes seem fine.
>>
>>> and also see comments embedded below.
>>
>> Follow up 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.
>>
>> Okay I see what you are trying to do now. We want overpass methods to 
>> follow the "if" path at 1201, but for them it should currently be a 
>> no-op. I'd be inclined to add in the assertion - the code is already 
>> concerned about not processing non-public overpasses with your 
>> proposed change to 1203. The assertion would ensure that anyone 
>> introducing a non-public overpass has it quickly drawn to their 
>> attention that doing so needs additional consideration.
>>
>> Thanks,
>> David
>> ----
>>
>>>>
>>>> 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