RFR (S): 8072588 - JVM crashes in JNI if toString is declared as an interface method

Wed May 27 13:17:35 UTC 2015

On 27/05/2015 11:13 PM, Staffan Larsen wrote:
>
>> On 27 maj 2015, at 15:12, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>> On 27/05/2015 11:02 PM, Stas Smirnov wrote:
>>> That's why I am asking, cause if all files for all tests will be just in
>>> a jni folder, it will be a real mess.
>>> That's why I do not understand why we can not group files in folder with
>>> a bug number as a name underneath "jni" folder to operate faster between
>>> the folders, otherwise developers must use some guideline on how to name
>>> the folders of their bugs.
>>
>> Use of bugs numbers was decided to be too obscure and that instead
>> functional descriptive names should be used. There's no hard and fast
>> rule on the granularity of grouping.
>
> See:
> https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests

That page is very vague and states:

"It is up to each group to organize the file structure within the 
corresponding component folder."

Looking at what has been done in different areas there are no hard and 
fast rules.

David
-----

>>
>> I don't have a preference as to whether these files sit in the jni
>> folder, or are placed in a further subfolder.
>>
>> David
>> -----
>>
>>> 27.05.2015 15:33, Andreas Eriksson пишет:
>>>> I'm not sure, I just find it easier to navigate multi-file tests if
>>>> they are grouped in a folder.
>>>> This test is one main file that is run by jtreg (ToStringTest.java)
>>>> and three other files used by it.
>>>> Stefan / David H, should I move all four test files to be in
>>>> test/runtime/jni/ directly?
>>>>
>>>> - Andreas
>>>>
>>>> On 2015-05-27 14:20, Stas Smirnov wrote:
>>>>> According to the new naming scheme all jni tests must be placed in
>>>>> one folder test/runtime/jni.
>>>>> Do I understand correctly, and it is missing on the wiki, that if
>>>>> there are several source files for the test, developer must group
>>>>> those files in a folder underneath the component folder using a
>>>>> specific name that reflects the test logic?
>>>>>
>>>>> 27.05.2015 12:01, Andreas Eriksson пишет:
>>>>>> Alright, thanks.
>>>>>>
>>>>>> I'll move the tests back to
>>>>>> test/runtime/jni/ToStringInInterfaceTest/ and get it pushed
>>>>>> tomorrow, unless anyone objects.
>>>>>>
>>>>>> - Andreas
>>>>>>
>>>>>> On 2015-05-27 10:48, Stefan Karlsson wrote:
>>>>>>> Hi Andreas,
>>>>>>>
>>>>>>> On 2015-05-27 10:44, Andreas Eriksson wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Thanks for looking at this.
>>>>>>>>
>>>>>>>> I'm getting some conflicting instructions on where the tests
>>>>>>>> should be placed.
>>>>>>>> According to Stas and David Simms they should be grouped by CR.
>>>>>>>> That was why I moved them from
>>>>>>>> test/runtime/jni/ToStringInInterfaceTest/ (webrev.01) to
>>>>>>>> test/runtime/jni/8072588/ (webrev.02).
>>>>>>>> Should I move them back? Or move them some other place?
>>>>>>>
>>>>>>> This was discussed some time ago and we decided to use descriptive
>>>>>>> names instead of CR numbers. See this page:
>>>>>>> https://wiki.openjdk.java.net/display/HotSpot/Naming+HotSpot+JTReg+Tests
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> StefanK
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Andreas
>>>>>>>>
>>>>>>>> On 2015-05-27 02:51, David Holmes wrote:
>>>>>>>>> Hi Andreas,
>>>>>>>>>
>>>>>>>>> We don't use bug numbers to name tests any more.
>>>>>>>>>
>>>>>>>>> Otherwise the change seems okay.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>> On 27/05/2015 1:10 AM, Andreas Eriksson wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Could a Reviewer please take a look at this?
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Andreas
>>>>>>>>>>
>>>>>>>>>> On 2015-05-20 14:03, David Simms wrote:
>>>>>>>>>>>
>>>>>>>>>>> Looks good
>>>>>>>>>>>
>>>>>>>>>>> /David Simms
>>>>>>>>>>>
>>>>>>>>>>> On 20/05/15 13:11, Andreas Eriksson wrote:
>>>>>>>>>>>> After some feedback from David and Stas the tests were moved.
>>>>>>>>>>>> Also,mMissing copyright headers were added.
>>>>>>>>>>>>
>>>>>>>>>>>> This is the latest webrev:
>>>>>>>>>>>> http://cr.openjdk.java.net/~aeriksso/8072588/webrev.02/
>>>>>>>>>>>>
>>>>>>>>>>>> Could a Reviewer please look at it?
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Andreas
>>>>>>>>>>>>
>>>>>>>>>>>> On 2015-05-06 12:52, Andreas Eriksson wrote:
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> David, I've added the native test to the test suite, could
>>>>>>>>>>>>> you take
>>>>>>>>>>>>> a look please?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I still need a Reviewer to take a look at this as well.
>>>>>>>>>>>>>
>>>>>>>>>>>>> New webrev:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~aeriksso/8072588/webrev.01/
>>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8072588
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Andreas
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2015-03-20 10:14, David Simms wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Looks good to me (not a 'R'-eviewer)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'd like to have your test checked in if possible. Native JNI
>>>>>>>>>>>>>> testing has just recently been added, we can talk about the
>>>>>>>>>>>>>> details
>>>>>>>>>>>>>> off-list.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> /David Simms
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 19/03/15 17:45, Andreas Eriksson wrote:
>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Could someone please take a look at this?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>> Andreas
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 2015-03-10 17:10, Andreas Eriksson wrote:
>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Please review this fix for a JNI issue in
>>>>>>>>>>>>>>>> jni_invoke_nonstatic.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The problem is that when toString is declared as an
>>>>>>>>>>>>>>>> interface
>>>>>>>>>>>>>>>> method it still has a vtable index, not an itable index as
>>>>>>>>>>>>>>>> jni_invoke_nonstatic expects.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This fix checks for a valid itable index instead of
>>>>>>>>>>>>>>>> checking if
>>>>>>>>>>>>>>>> the holder is an interface.
>>>>>>>>>>>>>>>> I also moved a duplicated check for JNI_VIRTUAL to be done
>>>>>>>>>>>>>>>> in one
>>>>>>>>>>>>>>>> check instead.
>>>>>>>>>>>>>>>> The change has been verified to fix the problem with a
>>>>>>>>>>>>>>>> small JNI
>>>>>>>>>>>>>>>> test, and has passed a jprt run with the hotspot testset.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Webrev:
>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~aeriksso/8072588/webrev.00/
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>>> Andreas
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>>
>>>>> Best regards,
>>>>> Stas Smirnov
>>>>>
>>>>> Stas Smirnov | Java VM Runtime SQE
>>>>> Phone: +7 812 3346130 | Mobile: +7 921 9262241
>>>>> Oracle Development SPB, LLC
>>>>> 10th Krasnoarmeyskaya 22A, St. Petersburg, 190103, Russia
>>>>>
>>>>
>>>
>>>
>>> --
>>>
>>> Best regards,
>>> Stas Smirnov
>>>
>>> Stas Smirnov | Java VM Runtime SQE
>>> Phone: +7 812 3346130 | Mobile: +7 921 9262241
>>> Oracle Development SPB, LLC
>>> 10th Krasnoarmeyskaya 22A, St. Petersburg, 190103, Russia
>>>
>