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

[coleen.phillimore at oracle.com]

Ok, like Karen said, ship it!
Coleen

On 9/28/17 1:05 PM, harold seigel wrote:
> Hi Coleen,
>
> I looked into moving the error message code into a function but there 
> wasn't enough commonality to make it worthwhile.  The new function 
> would require several parameters making it rather clunky.
>
> Thanks, Haorld
>
>
> On 9/27/2017 10: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?
>>
>> 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
>>>>>>>
>>>>>
>>>
>>
>