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

[harold seigel]

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
>>>>
>>