[foreign] RFR 8218144: Direct invoker does not handle varargs correctly

[Maurizio Cimadamore]

On 31/01/2019 15:53, Henry Jen wrote:
> Fix looks good, wondering why not just put down expected string in the test case instead of using String.format, which assumes format string is same for C and Java, while it maybe true, seems error prone to me.
The test used to do that before - but then I realized it was more 
obscure than anything. For the simple strings generated by the test, 
String.format and printf produce same results.
>
> Also I am curious if varargs would change any performance on the DNI?

As said in the RFR, I re-tested perfs after the patch and noticed no 
negative impact.

Maurizio

>
> Cheers,
> Henry
>
>
>> On Jan 31, 2019, at 6:10 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
>>
>> Hi,
>> our CI systems detected a spurious failures on StdLibTest, only on Linux x64. After a lot of digging, I figured out the problem was that SystemV ABI requires the RAX register to be set to the number of vector arguments in a variadic call:
>>
>> %rax: temporary register;with variable arguments passes information about the number of vector registers used
>>
>> So, if we are doing a variadic call, we need to set this register to the number of floating point args we're passing. The universal invoker seems to do that:
>>
>> http://hg.openjdk.java.net/panama/dev/file/tip/src/hotspot/cpu/x86/universalNativeInvoker_x86.cpp#l515
>>
>> For both variadic and non-variadic calls.
>>
>> So, a natural fix for the direct approach is to tweak the cast to the function pointers in our native stubs - e.g. from this:
>>
>> void DNI_invokeNative_V_JD(JNIEnv *env, jobject _unused, jlong addr, jlong arg0, jdouble arg1) {
>>       ((void (*)(jlong, jdouble))addr)(arg0, arg1);
>> }
>>
>> To this:
>>
>> void DNI_invokeNative_V_JD(JNIEnv *env, jobject _unused, jlong addr, jlong arg0, jdouble arg1) {
>>       ((void (*)(jlong, jdouble,...))addr)(arg0, arg1);
>> }
>>
>> Which will generate the right set for RAX (even though it will do so also for non-variadic call - but  that's the same as what universal invoker does).
>>
>> I also strengthened the test - I tried first to use sprintf instead of printf, but that didn't work, as that doesn't seem to reproduce the issue (my suspicion here is that 'printf' is optimized to look into RAX). So, instead, I tweaked the test to run for a lot longer, by repeatedly shuffling the permutation array, and I also increased the decimal length of the double formatting string - that's because, unfortunately, we were using 1.23, which has 4 chars, and sometimes the result of printf ended up with "-NaN" which also has 4 chars, despite being very wrong. With both measures, tests are much more likely, so much so that in the affected machine I've never been able to get a pass w/o the proposed patch.
>>
>> I verified that the patch works on all platforms, and I also verified that the patch does not introduce performance regressions, which is not the case (kind of expected, given that the only difference for adding a "..." is the extra MOV to RAX, as shown here [1]).
>>
>> Webrev:
>>
>> http://cr.openjdk.java.net/~mcimadamore/panama/8218144/
>>
>> Cheers
>> Maurizio
>>
>> [1] - https://gcc.godbolt.org/z/VAgfZc
>>
>>
>>
>>
>>