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

David Holmes david.holmes at oracle.com
Wed Sep 27 21:43:59 UTC 2017


On 28/09/2017 12:26 AM, coleen.phillimore at oracle.com wrote:
> 
> Harold,
> 
> Since you changed both instances of the mostly duplicated error message, 
> would this be a good time to make it a function?

There are similarities in the error messages but they are not the same.

David

> thanks,
> Coleen
> 
> On 9/27/17 9:38 AM, 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.
>>
>> 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