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

[Jorn Vernee]

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