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

[Sundararajan Athijegannathan]

Looks good.

-Sundar

On 31/01/19, 7:40 PM, Maurizio Cimadamore 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
>
>
>
>
>