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

[Jorn Vernee]

I see, that's what you mean by 'erasure'. I was thinking about type 
erasure in the classical sense (i.e. losing the information that it's 
actually a `long double`, by using the more generic carrier type 
`double`).

Sorry,
Jorn

Maurizio Cimadamore schreef op 2018-11-05 14:48:
> Sure,
> using double will generate truncation. This is an intended effect, in
> the sense that the 'erasure' to double is supposed to be something
> that we add as an interim step while we figure out a fuller story (as
> I said, possibly involving pseudo-value types carriers) which provides
> for better support.
> 
> Maurizio
> 
> On 05/11/2018 12:46, Jorn Vernee wrote:
>> 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