Re: [10] JBS: 8167408: Invalid critical JNI function lookup

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

Re: [10] JBS: 8167408: Invalid critical JNI function lookup

dean.long
Much better.  But why does the test need -Xbatch -Xcomp?  To me this
doesn't look like a compiler-only issue.  The test should give the same
result with -Xint right?  Adding runtime alias for their input...

dl


On 10/31/17 12:37 PM, jamsheed wrote:

> Hi Dean,
>
> Thank you for the review,
>
> tested with a test case, previously it was not working for
> windows-x86, now it works.
>
> revised webrev with test
> case:http://cr.openjdk.java.net/~jcm/8167408/webrev.01/
>
> Best regards,
>
> Jamsheed
>
>
> On Tuesday 31 October 2017 02:18 AM, [hidden email] wrote:
>> I think you need a native test for Windows x86 that defines
>> JavaCritical methods with various signatures (especially arrays) to
>> make sure this is working correctly.
>>
>> dl
>>
>>
>> On 10/30/17 9:45 AM, jamsheed wrote:
>>> Hi,
>>>
>>> request for review,
>>>
>>> jbs : https://bugs.openjdk.java.net/browse/JDK-8167408
>>>
>>> webrev: http://cr.openjdk.java.net/~jcm/8167408/webrev.00/
>>>
>>> (contributed by Ioannis Tsakpinis)
>>>
>>> desc:
>>>
>>> -- it starts with JavaCritical_ instead of Java_;
>>> -- it does not have extra JNIEnv* and jclass arguments;
>>> -- Java arrays are passed in two arguments: the first is an array
>>> length, and the second is a pointer to raw array data. That is, no
>>> need to call GetArrayElements and friends, you can instantly use a
>>> direct array pointer.
>>>
>>> updated arg_size calculation wrt above points.
>>>
>>> Best regards,
>>>
>>> Jamsheed
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [10] JBS: 8167408: Invalid critical JNI function lookup

David Holmes
On 1/11/2017 1:03 PM, [hidden email] wrote:
> Much better.  But why does the test need -Xbatch -Xcomp?  To me this
> doesn't look like a compiler-only issue.  The test should give the same
> result with -Xint right?  Adding runtime alias for their input...

AFAICS this should be a completely compiler and platform agnostic issue.
Good to add tests but they shouldn't be restricted to only 32-bit
Windows (especially as we no longer support it or do promoted builds for
it, or even build in Mach5!).

Thanks,
David

> dl
>
>
> On 10/31/17 12:37 PM, jamsheed wrote:
>> Hi Dean,
>>
>> Thank you for the review,
>>
>> tested with a test case, previously it was not working for
>> windows-x86, now it works.
>>
>> revised webrev with test
>> case:http://cr.openjdk.java.net/~jcm/8167408/webrev.01/
>>
>> Best regards,
>>
>> Jamsheed
>>
>>
>> On Tuesday 31 October 2017 02:18 AM, [hidden email] wrote:
>>> I think you need a native test for Windows x86 that defines
>>> JavaCritical methods with various signatures (especially arrays) to
>>> make sure this is working correctly.
>>>
>>> dl
>>>
>>>
>>> On 10/30/17 9:45 AM, jamsheed wrote:
>>>> Hi,
>>>>
>>>> request for review,
>>>>
>>>> jbs : https://bugs.openjdk.java.net/browse/JDK-8167408
>>>>
>>>> webrev: http://cr.openjdk.java.net/~jcm/8167408/webrev.00/
>>>>
>>>> (contributed by Ioannis Tsakpinis)
>>>>
>>>> desc:
>>>>
>>>> -- it starts with JavaCritical_ instead of Java_;
>>>> -- it does not have extra JNIEnv* and jclass arguments;
>>>> -- Java arrays are passed in two arguments: the first is an array
>>>> length, and the second is a pointer to raw array data. That is, no
>>>> need to call GetArrayElements and friends, you can instantly use a
>>>> direct array pointer.
>>>>
>>>> updated arg_size calculation wrt above points.
>>>>
>>>> Best regards,
>>>>
>>>> Jamsheed
>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [10] JBS: 8167408: Invalid critical JNI function lookup

David Holmes
On 1/11/2017 1:11 PM, David Holmes wrote:
> On 1/11/2017 1:03 PM, [hidden email] wrote:
>> Much better.  But why does the test need -Xbatch -Xcomp?  To me this
>> doesn't look like a compiler-only issue.  The test should give the
>> same result with -Xint right?  Adding runtime alias for their input...
>
> AFAICS this should be a completely compiler and platform agnostic issue.

Platform agnostic yes, but the test fails without -Xcomp (doesn't seem
to need -Xbatch). So there's something about this critical function
mechanism that I don't understand.

That said the test needs more logging so that if it does fail you can
see what, if anything got executed. So the non-critical versions of the
methods should print that they were called, and for good measure also
the critical versions.

Thanks,
David

Reply | Threaded
Open this post in threaded view
|

Re: [10] JBS: 8167408: Invalid critical JNI function lookup

Vladimir Ivanov
No magic here: the test isn't designed to be executed in interpreter and
always expect LookUp::test() to be compiled first.

Normal JNI entries are empty, but the test checks that the first element
is written to on the first invocation.

I agree that the bug is specific to JIT-compilers, since critical entry
points (JavaCritical_) are called only from compiled code. Interpreter
always goes through ordinary native counterparts (Java_).

IMO using -Xcomp is fine to force the methods to be compiled. -Xbatch is
redundant in that case.

Best regards,
Vladimir Ivanov

On 11/1/17 7:46 AM, David Holmes wrote:

> On 1/11/2017 1:11 PM, David Holmes wrote:
>> On 1/11/2017 1:03 PM, [hidden email] wrote:
>>> Much better.  But why does the test need -Xbatch -Xcomp?  To me this
>>> doesn't look like a compiler-only issue.  The test should give the
>>> same result with -Xint right?  Adding runtime alias for their input...
>>
>> AFAICS this should be a completely compiler and platform agnostic issue.
>
> Platform agnostic yes, but the test fails without -Xcomp (doesn't seem
> to need -Xbatch). So there's something about this critical function
> mechanism that I don't understand.
>
> That said the test needs more logging so that if it does fail you can
> see what, if anything got executed. So the non-critical versions of the
> methods should print that they were called, and for good measure also
> the critical versions.
>
> Thanks,
> David
>