java-native-access / jna

Java Native Access
Other
8.54k stars 1.68k forks source link

Should JNA promote float vararg arguments to double? #463

Closed marco2357 closed 8 years ago

marco2357 commented 9 years ago

C seems to require promoting of float vararg arguments to double:

include <stdarg.h>

void foo(int bar, ...) { va_list baz; va_arg(baz, float); }

GCC says: warning: ‘float’ is promoted to ‘double’ when passed through ‘...’ note: (so you should pass ‘double’ not ‘float’ to ‘va_arg’) note: if this code is reached, the program will abort

JNA doesn't promote vararg floats. Should users do this themselves or should JNA be changed? I think it would be better if JNA was handling this because it's very hard to debug. What do the JNA maintainers think?

twall commented 9 years ago

Can you determine if this is part of the C spec or part of GCC’s varargs handling?

On Jul 4, 2015, at 3:54 PM, marco2357 notifications@github.com wrote:

C seems to require promoting of float vararg arguments to double:

include

void foo(int bar, ...) { va_list baz; va_arg(baz, float); }

GCC says: warning: ‘float’ is promoted to ‘double’ when passed through ‘...’ note: (so you should pass ‘double’ not ‘float’ to ‘va_arg’) note: if this code is reached, the program will abort

JNA doesn't promote vararg floats. Should users do this themselves or should JNA be changed? I think it would be better if JNA was handling this because it's very hard to debug. What do the JNA maintainers think?

— Reply to this email directly or view it on GitHub.

marco2357 commented 9 years ago

Looks like it's part of the C spec:

Confirmed with:

From what I can see, JNA flattens the varargs into the regular argument array that it then passes to libffi. Libffi takes care of the promotion of char and short (because it applies to regular arguments and vararg arguments). But only floats in varargs need to be promoted, not regular arguments. So libffi (respectively the native part of JNA) can't differentiate between those two cases. So I suggest that JNA handles the promotion while flattening the varargs:

diff --git a/src/com/sun/jna/Function.java b/src/com/sun/jna/Function.java
index 7bcc68c..88121db 100644
--- a/src/com/sun/jna/Function.java
+++ b/src/com/sun/jna/Function.java
@@ -762,6 +762,11 @@
             Class argType = lastArg != null ? lastArg.getClass() : null;
             if (argType != null && argType.isArray()) {
                 Object[] varArgs = (Object[])lastArg;
+                for(int i = 0; i < varArgs.length; i++) {
+                    if(varArgs[i] instanceof Float) {
+                        varArgs[i] = (double)(Float)varArgs[i];
+                    }
+                }
                 Object[] fullArgs = new Object[inArgs.length+varArgs.length];
                 System.arraycopy(inArgs, 0, fullArgs, 0, inArgs.length-1);
                 System.arraycopy(varArgs, 0, fullArgs, inArgs.length-1, varArgs.length);
twall commented 9 years ago

Sounds reasonable, please provide a suitable PR and associated tests.

On Jul 6, 2015, at 8:01 AM, marco2357 notifications@github.com wrote:

Looks like it's part of the C spec:

http://c-faq.com/varargs/float.html • Explanation: http://stackoverflow.com/questions/11270588/variadic-function-va-arg-doesnt-work-with-float Confirmed with:

• gcc on Linux (64bit and 32bit) • gcc on cygwin and mingw (32bit) • Microsoft C/C++ compiler (64bit and 32bit) From what I can see, JNA flattens the varargs into the regular argument array that it then passes to libffi. Libffi takes care of the promotion of char and short (because it applies to regular arguments and vararg arguments). But only floats in varargs need to be promoted, not regular arguments. So libffi (respectively the native part of JNA) can't differentiate between those two cases. So I suggest that JNA handles the promotion while flattening the varargs:

diff --git a/src/com/sun/jna/Function.java b/src/com/sun/jna/Function.java index 7bcc68c..88121db 100644 --- a/src/com/sun/jna/Function.java +++ b/src/com/sun/jna/Function.java @@ -762,6 +762,11 @@ Class argType = lastArg != null ? lastArg.getClass() : null; if (argType != null && argType.isArray()) { Object[] varArgs = (Object[])lastArg;

  • for(int i = 0; i < varArgs.length; i++) {
  • if(varArgs[i] instanceof Float) {
  • varArgs[i] = (double)(Float)varArgs[i];
  • }
  • } Object[] fullArgs = new Object[inArgs.length+varArgs.length]; System.arraycopy(inArgs, 0, fullArgs, 0, inArgs.length-1); System.arraycopy(varArgs, 0, fullArgs, inArgs.length-1, varArgs.length);

— Reply to this email directly or view it on GitHub.

marco2357 commented 9 years ago

I'm sitting on a couple of JNA improvements and fixes. I'm very busy currently but will try to make PRs in the near future. Thanks!

umjammer commented 2 years ago

could you make this functionality pluggable?

this breaks objective-c 's objc_msgSend call with float arguments on intel 64 mac.


obj-c side

https://github.com/umjammer/rococoa/blob/0.8.5/rococoa-core/src/main/native/test.m#L121-L125

java side

https://github.com/umjammer/rococoa/blob/0.8.5/rococoa-core/src/test/java/org/rococoa/FoundationStructureReturnTest.java#L127-L136

log

main    Foundation.send - sending (boolean) [ID 0x7ff0e3c62650].testGetFloatByValue:(3.14:Float)  # <---- still float
main    Foundation.send - @@@0: 3.14:Float
java.lang.Exception: @@@1:  boolean:Class, [ID 0x7ff0e3c62650]:ID, [Selector testGetFloatByValue:]:Selector, 3.140000104904175:Double, null   # <---- changed as double!!!
    at org.rococoa.internal.MsgSendHandler.invoke(MsgSendHandler.java:122)
    at com.sun.jna.Library$Handler.invoke(Library.java:263)
    at com.sun.proxy.$Proxy13.syntheticSendMessage(Unknown Source)
    at org.rococoa.Foundation.send(Foundation.java:242)
    at org.rococoa.Foundation.send(Foundation.java:220)
    at org.rococoa.FoundationStructureReturnTest.test2(FoundationStructureReturnTest.java:132)
36893488147419103232.0                  # <--- printf shows passed float value is broken

org.opentest4j.AssertionFailedError: 
Expected :true
Actual   :false

i traced around before call below using debugger

at com.sun.jna.Library$Handler.invoke(Library.java:263)

then i found https://github.com/java-native-access/jna/blob/3705b849892aa3c37e5608e640eff19047811a5c/src/com/sun/jna/Function.java#L776 do this.


is this patch really common things? is my obj-c case rare?

on 28 Jun 2021 does he say same thing?