RFR 8154587: Resolution fails for default method named 'clone'

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8154587: Resolution fails for default method named 'clone'

harold seigel
Hi,

Please review this fix for JDK-8154587.  The fix adds additional special
casing to skip over non-public methods in class java.lang.Object during
default method and itable processing for interfaces.  These methods need
to be skipped over because of the interface method resolution rules in
JVM Spec 9, section 5.4.3.4
<https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.4>:

    3. Otherwise, if the class|Object|declares a method with the name
    and descriptor specified by the interface method reference, which
    has its|ACC_PUBLIC|flag set and does not have its|ACC_STATIC|flag
    set, method lookup succeeds.

Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8154587/webrev/

JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8154587

The fix was tested with JCK lang and VM tests, JTReg hotspot and many
JTReg JDK tests, M5 tier1 - tier5 tests, and JPRT.

Thanks, Harold

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8154587: Resolution fails for default method named 'clone'

David Holmes
Hi Harold,

Can't review the details here as not familiar enough with default method
rules, but one comment ...

On 1/12/2017 2:15 AM, harold seigel wrote:

> Hi,
>
> Please review this fix for JDK-8154587.  The fix adds additional special
> casing to skip over non-public methods in class java.lang.Object during
> default method and itable processing for interfaces.  These methods need
> to be skipped over because of the interface method resolution rules in
> JVM Spec 9, section 5.4.3.4
> <https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.4>:
>
>
>     3. Otherwise, if the class|Object|declares a method with the name
>     and descriptor specified by the interface method reference, which
>     has its|ACC_PUBLIC|flag set and does not have its|ACC_STATIC|flag
>     set, method lookup succeeds.
>
> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8154587/webrev/

... I think in

static bool is_nonpublic_jlo_method(Method* m)

we can spell "jlo" as "Object" (e.g as with Object_klass and
Object_klass_loaded()).

Thanks,
David

> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8154587
>
> The fix was tested with JCK lang and VM tests, JTReg hotspot and many
> JTReg JDK tests, M5 tier1 - tier5 tests, and JPRT.
>
> Thanks, Harold
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8154587: Resolution fails for default method named 'clone'

harold seigel
Thanks David.

I updated the webrev with that change.

Harold

On 12/3/2017 3:49 AM, David Holmes wrote:

> Hi Harold,
>
> Can't review the details here as not familiar enough with default
> method rules, but one comment ...
>
> On 1/12/2017 2:15 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this fix for JDK-8154587.  The fix adds additional
>> special casing to skip over non-public methods in class
>> java.lang.Object during default method and itable processing for
>> interfaces.  These methods need to be skipped over because of the
>> interface method resolution rules in JVM Spec 9, section 5.4.3.4
>> <https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.4>:
>>
>>
>>     3. Otherwise, if the class|Object|declares a method with the name
>>     and descriptor specified by the interface method reference, which
>>     has its|ACC_PUBLIC|flag set and does not have its|ACC_STATIC|flag
>>     set, method lookup succeeds.
>>
>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8154587/webrev/
>
> ... I think in
>
> static bool is_nonpublic_jlo_method(Method* m)
>
> we can spell "jlo" as "Object" (e.g as with Object_klass and
> Object_klass_loaded()).
>
> Thanks,
> David
>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8154587
>>
>> The fix was tested with JCK lang and VM tests, JTReg hotspot and many
>> JTReg JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>
>> Thanks, Harold
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8154587: Resolution fails for default method named 'clone'

Lois Foltan
In reply to this post by harold seigel
Hi Harold,

Fix looks good.  I would like to see three more test cases:

     - the same test written for "finalize" since that is also a
non-public method of Object.
     - a test where class C does not define the method "clone", an IAE
should result, correct?
     - a test case where I2 defines a public method of Object

Thanks,
Lois

On 11/30/2017 11:15 AM, harold seigel wrote:

> Hi,
>
> Please review this fix for JDK-8154587.  The fix adds additional
> special casing to skip over non-public methods in class
> java.lang.Object during default method and itable processing for
> interfaces.  These methods need to be skipped over because of the
> interface method resolution rules in JVM Spec 9, section 5.4.3.4
> <https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.4>:
>
>    3. Otherwise, if the class|Object|declares a method with the name
>    and descriptor specified by the interface method reference, which
>    has its|ACC_PUBLIC|flag set and does not have its|ACC_STATIC|flag
>    set, method lookup succeeds.
>
> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8154587/webrev/
>
> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8154587
>
> The fix was tested with JCK lang and VM tests, JTReg hotspot and many
> JTReg JDK tests, M5 tier1 - tier5 tests, and JPRT.
>
> Thanks, Harold
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8154587: Resolution fails for default method named 'clone'

harold seigel
Hi Lois,

Thanks for the review.  I'll add these tests in the next iteration of
this webrev.

 >> - a test where class C does not define the method "clone", an IAE
should result, correct?

Yes, the test throws an IAE.

Harold


On 12/4/2017 2:31 PM, Lois Foltan wrote:

> Hi Harold,
>
> Fix looks good.  I would like to see three more test cases:
>
>     - the same test written for "finalize" since that is also a
> non-public method of Object.
>     - a test where class C does not define the method "clone", an IAE
> should result, correct?
>     - a test case where I2 defines a public method of Object
>
> Thanks,
> Lois
>
> On 11/30/2017 11:15 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this fix for JDK-8154587.  The fix adds additional
>> special casing to skip over non-public methods in class
>> java.lang.Object during default method and itable processing for
>> interfaces.  These methods need to be skipped over because of the
>> interface method resolution rules in JVM Spec 9, section 5.4.3.4
>> <https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.4>:
>>
>>    3. Otherwise, if the class|Object|declares a method with the name
>>    and descriptor specified by the interface method reference, which
>>    has its|ACC_PUBLIC|flag set and does not have its|ACC_STATIC|flag
>>    set, method lookup succeeds.
>>
>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8154587/webrev/
>>
>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8154587
>>
>> The fix was tested with JCK lang and VM tests, JTReg hotspot and many
>> JTReg JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>
>> Thanks, Harold
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8154587: Resolution fails for default method named 'clone'

harold seigel
Hi,

Please review this updated webrev for bug JDK-8154587:

    http://cr.openjdk.java.net/~hseigel/bug_8154587.2/webrev/

The main differences from the previous webrev are:

 1. Improved comments and code changes to klassVtable.cpp and
    klassVtable.hpp
 2. Added an assert to cpCache.cpp
 3. Added new tests.  The purpose of many these tests, including the
    .../clone/invokevirtual/ tests, is to check for regressions.

These changes were regression tested with the tests listed below plus
tonga tests.

Thanks, Harold

On 12/6/2017 8:49 AM, harold seigel wrote:

> Hi Lois,
>
> Thanks for the review.  I'll add these tests in the next iteration of
> this webrev.
>
> >> - a test where class C does not define the method "clone", an IAE
> should result, correct?
>
> Yes, the test throws an IAE.
>
> Harold
>
>
> On 12/4/2017 2:31 PM, Lois Foltan wrote:
>> Hi Harold,
>>
>> Fix looks good.  I would like to see three more test cases:
>>
>>     - the same test written for "finalize" since that is also a
>> non-public method of Object.
>>     - a test where class C does not define the method "clone", an IAE
>> should result, correct?
>>     - a test case where I2 defines a public method of Object
>>
>> Thanks,
>> Lois
>>
>> On 11/30/2017 11:15 AM, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this fix for JDK-8154587.  The fix adds additional
>>> special casing to skip over non-public methods in class
>>> java.lang.Object during default method and itable processing for
>>> interfaces.  These methods need to be skipped over because of the
>>> interface method resolution rules in JVM Spec 9, section 5.4.3.4
>>> <https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.4>:
>>>
>>>    3. Otherwise, if the class|Object|declares a method with the name
>>>    and descriptor specified by the interface method reference, which
>>>    has its|ACC_PUBLIC|flag set and does not have its|ACC_STATIC|flag
>>>    set, method lookup succeeds.
>>>
>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8154587/webrev/
>>>
>>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8154587
>>>
>>> The fix was tested with JCK lang and VM tests, JTReg hotspot and
>>> many JTReg JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>>
>>> Thanks, Harold
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8154587: Resolution fails for default method named 'clone'

Karen Kinnear
Harold,

Ship It!

Many many hanks for figuring out all the details of this corner case and for all the work
to write the additional tests.

Minor requests:

1) cpCache.cpp: can you change the assertion text to say “Calling non-public method in Object with invokeinterface” so
we don’t have to change it as confusing when we add nestmates and allow general calling of non-public methods in
invokeinterface

2. defaultHashCode.jasm
Summary: change “overloaded” to “local”

I don’t need to see it again.

thanks,
Karen

> On Dec 11, 2017, at 10:34 AM, harold seigel <[hidden email]> wrote:
>
> Hi,
>
> Please review this updated webrev for bug JDK-8154587:
>
>   http://cr.openjdk.java.net/~hseigel/bug_8154587.2/webrev/
>
> The main differences from the previous webrev are:
>
> 1. Improved comments and code changes to klassVtable.cpp and
>   klassVtable.hpp
> 2. Added an assert to cpCache.cpp
> 3. Added new tests.  The purpose of many these tests, including the
>   .../clone/invokevirtual/ tests, is to check for regressions.
>
> These changes were regression tested with the tests listed below plus tonga tests.
>
> Thanks, Harold
>
> On 12/6/2017 8:49 AM, harold seigel wrote:
>> Hi Lois,
>>
>> Thanks for the review.  I'll add these tests in the next iteration of this webrev.
>>
>> >> - a test where class C does not define the method "clone", an IAE should result, correct?
>>
>> Yes, the test throws an IAE.
>>
>> Harold
>>
>>
>> On 12/4/2017 2:31 PM, Lois Foltan wrote:
>>> Hi Harold,
>>>
>>> Fix looks good.  I would like to see three more test cases:
>>>
>>>     - the same test written for "finalize" since that is also a non-public method of Object.
>>>     - a test where class C does not define the method "clone", an IAE should result, correct?
>>>     - a test case where I2 defines a public method of Object
>>>
>>> Thanks,
>>> Lois
>>>
>>> On 11/30/2017 11:15 AM, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this fix for JDK-8154587.  The fix adds additional special casing to skip over non-public methods in class java.lang.Object during default method and itable processing for interfaces.  These methods need to be skipped over because of the interface method resolution rules in JVM Spec 9, section 5.4.3.4 <https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.4>:
>>>>
>>>>    3. Otherwise, if the class|Object|declares a method with the name
>>>>    and descriptor specified by the interface method reference, which
>>>>    has its|ACC_PUBLIC|flag set and does not have its|ACC_STATIC|flag
>>>>    set, method lookup succeeds.
>>>>
>>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8154587/webrev/
>>>>
>>>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8154587
>>>>
>>>> The fix was tested with JCK lang and VM tests, JTReg hotspot and many JTReg JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>>>
>>>> Thanks, Harold
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8154587: Resolution fails for default method named 'clone'

harold seigel
Thanks Karen for the review and for all your help!

Harold

On 12/11/2017 12:58 PM, Karen Kinnear wrote:

> Harold,
>
> Ship It!
>
> Many many hanks for figuring out all the details of this corner case and for all the work
> to write the additional tests.
>
> Minor requests:
>
> 1) cpCache.cpp: can you change the assertion text to say “Calling non-public method in Object with invokeinterface” so
> we don’t have to change it as confusing when we add nestmates and allow general calling of non-public methods in
> invokeinterface
>
> 2. defaultHashCode.jasm
> Summary: change “overloaded” to “local”
>
> I don’t need to see it again.
>
> thanks,
> Karen
>
>> On Dec 11, 2017, at 10:34 AM, harold seigel <[hidden email]> wrote:
>>
>> Hi,
>>
>> Please review this updated webrev for bug JDK-8154587:
>>
>>    http://cr.openjdk.java.net/~hseigel/bug_8154587.2/webrev/
>>
>> The main differences from the previous webrev are:
>>
>> 1. Improved comments and code changes to klassVtable.cpp and
>>    klassVtable.hpp
>> 2. Added an assert to cpCache.cpp
>> 3. Added new tests.  The purpose of many these tests, including the
>>    .../clone/invokevirtual/ tests, is to check for regressions.
>>
>> These changes were regression tested with the tests listed below plus tonga tests.
>>
>> Thanks, Harold
>>
>> On 12/6/2017 8:49 AM, harold seigel wrote:
>>> Hi Lois,
>>>
>>> Thanks for the review.  I'll add these tests in the next iteration of this webrev.
>>>
>>>>> - a test where class C does not define the method "clone", an IAE should result, correct?
>>> Yes, the test throws an IAE.
>>>
>>> Harold
>>>
>>>
>>> On 12/4/2017 2:31 PM, Lois Foltan wrote:
>>>> Hi Harold,
>>>>
>>>> Fix looks good.  I would like to see three more test cases:
>>>>
>>>>      - the same test written for "finalize" since that is also a non-public method of Object.
>>>>      - a test where class C does not define the method "clone", an IAE should result, correct?
>>>>      - a test case where I2 defines a public method of Object
>>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>> On 11/30/2017 11:15 AM, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this fix for JDK-8154587.  The fix adds additional special casing to skip over non-public methods in class java.lang.Object during default method and itable processing for interfaces.  These methods need to be skipped over because of the interface method resolution rules in JVM Spec 9, section 5.4.3.4 <https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.4>:
>>>>>
>>>>>     3. Otherwise, if the class|Object|declares a method with the name
>>>>>     and descriptor specified by the interface method reference, which
>>>>>     has its|ACC_PUBLIC|flag set and does not have its|ACC_STATIC|flag
>>>>>     set, method lookup succeeds.
>>>>>
>>>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8154587/webrev/
>>>>>
>>>>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8154587
>>>>>
>>>>> The fix was tested with JCK lang and VM tests, JTReg hotspot and many JTReg JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>>>>
>>>>> Thanks, Harold
>>>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8154587: Resolution fails for default method named 'clone'

Lois Foltan
In reply to this post by harold seigel
Looks good!  Thanks for all the additional testing.
Lois

On 12/11/2017 10:34 AM, harold seigel wrote:

> Hi,
>
> Please review this updated webrev for bug JDK-8154587:
>
>    http://cr.openjdk.java.net/~hseigel/bug_8154587.2/webrev/
>
> The main differences from the previous webrev are:
>
> 1. Improved comments and code changes to klassVtable.cpp and
>    klassVtable.hpp
> 2. Added an assert to cpCache.cpp
> 3. Added new tests.  The purpose of many these tests, including the
>    .../clone/invokevirtual/ tests, is to check for regressions.
>
> These changes were regression tested with the tests listed below plus
> tonga tests.
>
> Thanks, Harold
>
> On 12/6/2017 8:49 AM, harold seigel wrote:
>> Hi Lois,
>>
>> Thanks for the review.  I'll add these tests in the next iteration of
>> this webrev.
>>
>> >> - a test where class C does not define the method "clone", an IAE
>> should result, correct?
>>
>> Yes, the test throws an IAE.
>>
>> Harold
>>
>>
>> On 12/4/2017 2:31 PM, Lois Foltan wrote:
>>> Hi Harold,
>>>
>>> Fix looks good.  I would like to see three more test cases:
>>>
>>>     - the same test written for "finalize" since that is also a
>>> non-public method of Object.
>>>     - a test where class C does not define the method "clone", an
>>> IAE should result, correct?
>>>     - a test case where I2 defines a public method of Object
>>>
>>> Thanks,
>>> Lois
>>>
>>> On 11/30/2017 11:15 AM, harold seigel wrote:
>>>> Hi,
>>>>
>>>> Please review this fix for JDK-8154587.  The fix adds additional
>>>> special casing to skip over non-public methods in class
>>>> java.lang.Object during default method and itable processing for
>>>> interfaces.  These methods need to be skipped over because of the
>>>> interface method resolution rules in JVM Spec 9, section 5.4.3.4
>>>> <https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.4>:
>>>>
>>>>    3. Otherwise, if the class|Object|declares a method with the name
>>>>    and descriptor specified by the interface method reference, which
>>>>    has its|ACC_PUBLIC|flag set and does not have its|ACC_STATIC|flag
>>>>    set, method lookup succeeds.
>>>>
>>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8154587/webrev/
>>>>
>>>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8154587
>>>>
>>>> The fix was tested with JCK lang and VM tests, JTReg hotspot and
>>>> many JTReg JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>>>
>>>> Thanks, Harold
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8154587: Resolution fails for default method named 'clone'

harold seigel
Thanks Lois!

Harold

On 12/11/2017 1:52 PM, Lois Foltan wrote:

> Looks good!  Thanks for all the additional testing.
> Lois
>
> On 12/11/2017 10:34 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this updated webrev for bug JDK-8154587:
>>
>>    http://cr.openjdk.java.net/~hseigel/bug_8154587.2/webrev/
>>
>> The main differences from the previous webrev are:
>>
>> 1. Improved comments and code changes to klassVtable.cpp and
>>    klassVtable.hpp
>> 2. Added an assert to cpCache.cpp
>> 3. Added new tests.  The purpose of many these tests, including the
>>    .../clone/invokevirtual/ tests, is to check for regressions.
>>
>> These changes were regression tested with the tests listed below plus
>> tonga tests.
>>
>> Thanks, Harold
>>
>> On 12/6/2017 8:49 AM, harold seigel wrote:
>>> Hi Lois,
>>>
>>> Thanks for the review.  I'll add these tests in the next iteration
>>> of this webrev.
>>>
>>> >> - a test where class C does not define the method "clone", an IAE
>>> should result, correct?
>>>
>>> Yes, the test throws an IAE.
>>>
>>> Harold
>>>
>>>
>>> On 12/4/2017 2:31 PM, Lois Foltan wrote:
>>>> Hi Harold,
>>>>
>>>> Fix looks good.  I would like to see three more test cases:
>>>>
>>>>     - the same test written for "finalize" since that is also a
>>>> non-public method of Object.
>>>>     - a test where class C does not define the method "clone", an
>>>> IAE should result, correct?
>>>>     - a test case where I2 defines a public method of Object
>>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>> On 11/30/2017 11:15 AM, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this fix for JDK-8154587.  The fix adds additional
>>>>> special casing to skip over non-public methods in class
>>>>> java.lang.Object during default method and itable processing for
>>>>> interfaces.  These methods need to be skipped over because of the
>>>>> interface method resolution rules in JVM Spec 9, section 5.4.3.4
>>>>> <https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.4.3.4>:
>>>>>
>>>>>    3. Otherwise, if the class|Object|declares a method with the name
>>>>>    and descriptor specified by the interface method reference, which
>>>>>    has its|ACC_PUBLIC|flag set and does not have its|ACC_STATIC|flag
>>>>>    set, method lookup succeeds.
>>>>>
>>>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8154587/webrev/
>>>>>
>>>>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8154587
>>>>>
>>>>> The fix was tested with JCK lang and VM tests, JTReg hotspot and
>>>>> many JTReg JDK tests, M5 tier1 - tier5 tests, and JPRT.
>>>>>
>>>>> Thanks, Harold
>>>>>
>>>>
>>>
>>
>