[foreign] RFR 8211722: Add comprehensive ABI test for downcalls

[Maurizio Cimadamore]

Updated webrev:

http://cr.openjdk.java.net/~mcimadamore/panama/8211722_v2

This new version adds testing for upcalls too.

When writing the new upcall tests I realized that the downcall test was 
defeated (at least parts of it) by the O3 optimizations used when 
compiling the test libraries. As a result some of the library test code 
was not executed (as GCC could see that the body had no effect).

Turns out there was a biggie issue with 
DIrectNativeInvoker/UpcallHandler: while it is safe to erase all 
integral values to 'long', it is not safe to erase all floating points 
to 'double' (as the representation of float and double is different, so 
accessing lowest 32 bits of the MMX register doesn't make sense and 
yields wrong value).

So, this patch also disables direct approach when used with 'floats'.

I also found a way to significantly up the numbers of checks carried out 
- now the generators break up the generated functions into chunks:

f0_F
f0_P
...
f0_S
f1_F
f1_P
...
f1_S

Then, the test uses a bunch of regex filter to tell jextract not to 
extract everything at once. This way we can make sure that we can always 
split the test in a suitable way so that jextract will be ok.

This allowed me to test with 3 parameters and 3 struct fields, which 
allows us to test structs that are 'too big' to fit in registers (e.g. 
passed in memory). I also added both 'double' and 'float' to the kind of 
struct field/parameter type that can be generated (in the last patch 
only 'float' was used).

Please give it a try on Mac, as the generated c file use some pragmas to 
make sure optimizations are disabled. I used that in combination with 
#ifdef to avoid warnings about unrecognized pragmas, but better to 
double check.

Cheers
Maurizio


On 04/10/18 17:39, Maurizio Cimadamore wrote:
> Hi,
> since we are now supporting multiple invocation schemes (for both 
> upcalls and downcalls) I thought it would be a good idea to start 
> testing the ABI code more in depth. I have created a generator which 
> creates an header/C file with aroun 2K different functions in it, each 
> of which is a specific permutation of a fixes set of parameter types. 
> Example:
>
> void f_PI_V_(void* p0, int p1) ;
> void f_PF_V_(void* p0, float p1) ;
> void f_PP_V_(void* p0, void* p1) ;
>
> The implementation of these functions (in the C file) is derived in a 
> simple way:
>
> * if the function is void, body is empty
> * otherwise, it is assumed that the function will return its first 
> parameter
>
> For struct parameters the generator also generates several 
> combinations of struct fields, to make sure that everything works 
> correctly - here an example of the structs defined:
>
> struct S_II { int p0; int p1; };
> struct S_IF { int p0; float p1; };
> struct S_IP { int p0; void* p1; };
>
> To keep the number of combination under control, I had to limit the 
> function arity to 3, and the struct field arity to 2. Otherwise the 
> extracted @NativeHeader had a declaration string which was too big to 
> fit in the constant pool (!!).
>
> The test extracts the header, loads the extracted classes and binds 
> them. Then it uses reflection to call every single function in the 
> header. This means that for every function to be called, the test will 
> have to synthesize some parameter values to be passed to the function. 
> In addition to all this, the test also verifies that, if a non-void 
> function is called, the function call returns a value that is equal to 
> the first argument passed.
>
> Needless to say, this test uncovered several issues which have been 
> fixed as part of this patch:
>
> * The universal invoker doesn't like empty structs; there were several 
> crashes, as no 'binding' was generated from them. Looking at the 
> System V ABI it's not even 100% clear what should happen in case of an 
> empty struct passed by value, but after inspecting output of clang/gcc 
> it seems evident that empty structs are simply skipped (e.g. they do 
> not take any register, which makes sense). So, to support this, I had 
> to add support for empty structs in the classing sequence builder, and 
> then special case UniversalInvoker::boxValue and 
> UniversalInvoker::unboxValue, accordingly (to avoid touching pointers 
> whose layout is void).
>
> * There was another issue in UniversalInvoker: when a function 
> returned a struct by value using multi-register return,  the register 
> values were not copied in the correct order into the resulting struct. 
> To fix this I had to tweak CallingSequence to add 'offsets' for return 
> types and fix the code in UniversalInvoker::invoke to compute the 
> offset into the return register array correctly (the code was 
> erroneously computing the offset using ArgumentBinding::getOffset).
>
> * The direct invocation scheme was also broken when mixed long and 
> floating point parameters were passed; more specifically, the 
> nativeMethodType signature was not normalized (e.g. put into a form 
> where longs came first then followed by doubles) and as a result we 
> failed to lookup the right method handle trampoline. I have fixed the 
> direct shuffler in a more general way - now a nativeMethodType is 
> always 'normalized' and additionally I tweaked the way in which method 
> handle permutations were computed, as I realized that the permutation 
> step has to be applied at different steps depending on whether we are 
> inside an upcall or a downcall (and with different method type targets 
> too).
>
> The resulting code passes the new ABI test (and all other tests too of 
> course); note that the new ABI test is ran two times: once w/o any 
> invocation scheme parameter (meaning NativeInvoker is free to chose 
> fast path if it wants) and another which force the universal scheme. 
> This should provide maximum coverage for both schemes.
>
> I will work separately on a similar approach to test upcalls (but I 
> think it's better to split given that this one is already biggie).
>
> Webrev:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/8211722/
>
> Cheers
> Maurizio
>
>
>