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