RFR: jextract generates BigDecimal for "long double" but binder fails to handle it

[Jorn Vernee]

Actually, I'm confusing DoubleComplex with LongDouble when I say "use 2 
getters / setters for the component 'doubles'". I'm probably sending out 
these emails a little too quickly (and still waking up), sorry :/

But I think the truncation of `long double` is a real problem. The 
example doesn't work, but I hope you get the idea; If a value coming 
from native is larger than what fits into a `double`, the value will get 
truncated by `RF_longDoubleToDouble` [1].

Jorn

[1] : 
http://cr.openjdk.java.net/~mcimadamore/panama/8213029/src/hotspot/share/prims/references.cpp.html

Jorn Vernee schreef op 2018-11-05 13:46:
> Another tricky thing I just realized is that `long double` is only 8
> bytes in the MSx64 ABI.
> 
> For SysV, I'm wondering if the value doesn't get truncated if you use
> `double` as a carrier type. Consider the following C code:
> 
>     long double get_val() {
>         return (1 << 64); // does not fit in double
>     }
> 
>     void use_val(long double val) {
>         //... usage
>     }
> 
> On the Java side:
> 
>     @NativeHeader(...)
>     interface MyLib {
>         double get_val();
>         void use_val(double val);
>     }
> 
>     public static void main(String[] args) {
>         Mylib myLib = Libraries.bind(lookup(), "MyLib");
> 
>         double val = myLib.get_val(); // TRUNCATION!!
>         myLib.use_val(val);
>     }
> 
> It doesn't look like this case is currently being tested, since you're
> generating the values to test on the Java side, so they will all fit
> into `double` any ways.
> 
> Jorn
> 
> Jorn Vernee schreef op 2018-11-05 13:30:
>> Maurizio Cimadamore schreef op 2018-11-05 13:09:
>>> On 05/11/2018 11:47, Jorn Vernee wrote:
>>>> Hi,
>>>> 
>>>> I can't run the tests on this, but I thought I'd review any ways 
>>>> since I've spent a lot of time with the same code recently.
>>>> 
>>>> LongDoubleComplex.java:
>>>> 
>>>>     Not sure this diff should be in here, the rhs is what I already 
>>>> have locally.
>>> 
>>> Not sure what you mean - the tip of LongDoubleComplex is using 
>>> BigDecimal:
>>> 
>>> http://hg.openjdk.java.net/panama/dev/file/2e5e621e70d7/src/java.base/share/classes/java/foreign/memory/LongDoubleComplex.java
>>> 
>> 
>> Yep that was my bad, sorry.
>> 
>>>> 
>>>> Util.java:
>>>> 
>>>>     Somewhat confusing to do the get and set operations in one 
>>>> method, but that's up to you :)
>>>> 
>>>> - I think you've forgotten the diff for 
>>>> jdk/internal/foreign/abi/sysv/x64/StorageNames.java ?
>>> No, I just didn't change that file - is it used for debugging? I'll 
>>> check
>> 
>> It's used for debugging to output the register names chosen by
>> CallingSequenceBuilder:
>> 
>>     if (DEBUG) {
>>         System.out.println("Argument " + arg.getName() + " will be
>> passed in register " + StorageNames.getStorageName(storage));
>>     }
>> 
>> You've added new storage classes, so StorageNames.java needs to be
>> updated to recognize those new classes and return the right names.
>> 
>>>> 
>>>> - libcomplex_aux.c, libcomplex_aux.h, and libLongDouble.c are 
>>>> missing copyright headers.
>>>> 
>>>> Rest looks good to me.
>>>> 
>>>> On a general note: Did you consider rolling your own 
>>>> `Struct<LongDouble>` type as a carrier?
>>> 
>>> I think that's a possibility and yes, I've considered it, but it 
>>> needs
>>> to have enough methods on it so that people can do numeric 
>>> computation
>>> w/o projecting into a BigDecimal? And probably we'd like it to be a
>>> valhalla value type too...
>> 
>> Maybe, or you could leave it at just 2 getters and setter for the 2
>> component 'doubles'. I'd expect most usage to be on the native side
>> any ways, and you just need to have a way to have a handle to the
>> value on the Java side as well. But if someone really wanted to they
>> could access the whole value on the Java side as well (e.g. to print
>> it out for debugging purposes, not necessarily to do math with),
>> they'd be accepting the slow-path.
>> 
>> Could also just wait and see what the feedback is on this, and revisit
>> later if needed.
>> 
>> Jorn
>> 
>>> Maurizio
>>> 
>>>> 
>>>> Cheers,
>>>> Jorn
>>>> 
>>>> Maurizio Cimadamore schreef op 2018-11-05 11:27:
>>>>> Hi,
>>>>> this patch adds support for X87 values in the binder
>>>>> 
>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8213029/
>>>>> 
>>>>> The Hotspot changes are straightforward; there's a new X87 shuffle
>>>>> recipe category which needs to be taken into account (its size is 
>>>>> 128
>>>>> bits). Changes are needed on the binder side too as the X87 
>>>>> arguments
>>>>> have to be properly classified.
>>>>> 
>>>>> More specifically, the ABI is very subtle when it comes to 
>>>>> 'complex'
>>>>> values; they are generally passed in stack slots (if they are in
>>>>> argument position) but they are passed in registers (X87 st0 for 
>>>>> 'long
>>>>> double' or both 'st0' and 'st1' for 'long double complex') for 
>>>>> return
>>>>> values. To achieve this, we need to teach the binder about the 
>>>>> special
>>>>> LongDoubleComplex struct, so that the right classification occurs 
>>>>> (see
>>>>> changes to CallingSequenceBuilderImpl).
>>>>> 
>>>>> Another issue is how to handle X87 values in Java - I started out
>>>>> trying to map these values to BigDecimal, but after getting it to 
>>>>> work
>>>>> I realized that there were too many unanswered questions: one
>>>>> performance side, to decode an X87 value into a BigDecimal you have 
>>>>> to
>>>>> create lots of intermediate BigIntegers/BigDecimal; on the
>>>>> expressiveness side, BigDecimal is lacking support for 
>>>>> NaN/Infinity,
>>>>> making it an improper fit.
>>>>> 
>>>>> I then decided to 'erase' X87 values down to 'double' (using a JNI
>>>>> helper function to do the conversion, which should be efficient as 
>>>>> it
>>>>> can take advantage of platform specific instructions such as flstp
>>>>> etc.).
>>>>> 
>>>>> I tested it with fast-debug to make sure there were no weird 
>>>>> hotspot
>>>>> failures, but please give it a spin to make sure it all works.
>>>>> 
>>>>> I also updates existing complex tests to add more cases and upcall 
>>>>> support.
>>>>> 
>>>>> Cheers
>>>>> 
>>>>> Maurizio