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

[David Holmes]

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