[foreign] Improve error reporting when Java signature and native function layout don't match.

Tue Sep 25 18:33:15 UTC 2018

Hi Maurizio,
Thanks for looking at the patch.

I have made the check simpler as you suggested [1]. Doing carrier <-> 
layout checks in LayoutType makes sense to me.

If you appreciate this kind of contribution I had 2 more things I ran 
into;

1.) When a native function returns `null`, it seems that this value is 
always boxed using `References#ofVoid`, and when you de-reference it you 
get an UOE without a message. I'm not sure if boxing as Void is 
intentional, but either way it would be nice if it gave a reason like 
'can not de-reference null pointer'.

2.) The way NativeHeader#declarations is parsed makes it so that a 
faulty layout string like `exit(i32)v` (should be `exit=(i32)v`) is 
silently ignored, and you get an AbstractMethodError when invoking the 
method. Part of the problem I think, is that the parser splits the 
declarations string by `=`, and then goes from there. I'm thinking a 
more robust approach would be to make NativeHeader#declarations a 
String[] instead of a String, and then try to parse exactly 1 
declaration for each element. What do you think?

I could try and make patches for those later this week.

Jorn

[1] : 
https://gist.github.com/JornVernee/2fb68d70bad1aa807b50b77840117b27

Maurizio Cimadamore schreef op 2018-09-25 18:43:
> Hi Jorn,
> your patch looks good, but I think I would like it to just limit
> things at a basic arity check (and void vs. non void check) in the
> function. Given how we treat native types (layout + carrier), how
> carriers are assigned to layouts is up to the method signature - if
> somebody really wants to see a pointer as a float, so be it, although
> I agree it looks silly.
> 
> More generally, I think that if we go down the path of enforcing more
> checks, we probably want to enforce carrier vs. layout compatibility
> in a different place - that is LayoutType. That is the natural place
> where to rule out ill-formed combinations.
> 
> So there are two checks here: one is the one you needed, which checks
> that a method signature is compatible with its function layout (same
> arity, same varargness, same kind of return). The other is a more in
> depth check performed on LayoutType, which I think is rather
> orthogonal w.r.t. former. I'd suggest to go ahead with the first.
> 
> Makes sense?
> 
> Maurizio
> 
> 
> 
> On 25/09/18 14:27, Jorn Vernee wrote:
>> Hello,
>> 
>> Thanks for accepting my last contribution!
>> 
>> During testing I was running into an exception like this:
>> 
>>     Exception in thread "main" 
>> java.lang.ArrayIndexOutOfBoundsException: Index 0 out of bounds for 
>> length 0
>>     at 
>> java.base/jdk.internal.foreign.NativeInvoker.invokeNormal(NativeInvoker.java:216)
>>     at 
>> java.base/jdk.internal.foreign.NativeInvoker.invoke(NativeInvoker.java:200)
>>     at com.github.jdnl.Main$MyLib$Impl/0x000000010009c440.exit(Unknown 
>> Source)
>>       at com.github.jdnl.Main.main(Main.java:17)
>> 
>> The cause turned out to be a mismatch between the Java method 
>> signature and the native function layout:
>> 
>>     @NativeHeader(declarations =
>>         "exit=()v" // missing param
>>     )
>>     interface MyLib {
>>         void exit(int code);
>>     }
>> 
>> Since this exception is pretty cryptic, I'd like to contribute a patch 
>> that adds an explicit check for this to NativeInvoker, which does some 
>> basic verification that the two signatures match. With the patch I get 
>> this error instead (note the last 'Caused by'):
>> 
>>     Exception in thread "main" java.lang.RuntimeException: Failed to 
>> generate implementation for class interface com.github.jdnl.Main$MyLib
>>     at 
>> java.base/jdk.internal.foreign.LibrariesHelper.generateImpl(LibrariesHelper.java:65)
>>     at 
>> java.base/jdk.internal.foreign.LibrariesHelper$3.computeValue(LibrariesHelper.java:132)
>>     at 
>> java.base/jdk.internal.foreign.LibrariesHelper$3.computeValue(LibrariesHelper.java:124)
>>     at 
>> java.base/java.lang.ClassValue.getFromHashMap(ClassValue.java:226)
>>     at 
>> java.base/java.lang.ClassValue.getFromBackup(ClassValue.java:208)
>>     at java.base/java.lang.ClassValue.get(ClassValue.java:114)
>>     at 
>> java.base/jdk.internal.foreign.LibrariesHelper.getHeaderImplClass(LibrariesHelper.java:155)
>>     at 
>> java.base/jdk.internal.foreign.LibrariesHelper.bind(LibrariesHelper.java:241)
>>     at 
>> java.base/jdk.internal.foreign.LibrariesHelper.bind(LibrariesHelper.java:260)
>>     at java.base/java.foreign.Libraries.bind(Libraries.java:61)
>>     at com.github.jdnl.Main.main(Main.java:17)
>>     Caused by: java.lang.RuntimeException: Failed to generate method 
>> public abstract void com.github.jdnl.Main$MyLib.exit(int)
>>     at 
>> java.base/jdk.internal.foreign.BinderClassGenerator.generateMembers(BinderClassGenerator.java:153)
>>     at 
>> java.base/jdk.internal.foreign.HeaderImplGenerator.generateMembers(HeaderImplGenerator.java:103)
>>     at 
>> java.base/jdk.internal.foreign.BinderClassGenerator.generate(BinderClassGenerator.java:103)
>>     at 
>> java.base/jdk.internal.foreign.LibrariesHelper.lambda$generateImpl$0(LibrariesHelper.java:62)
>>     at java.base/java.security.AccessController.doPrivileged(Native 
>> Method)
>>     at 
>> java.base/jdk.internal.foreign.LibrariesHelper.generateImpl(LibrariesHelper.java:61)
>>     ... 10 more
>>     Caused by: java.lang.IllegalArgumentException: Java method 
>> signature and native layout not compatible: public abstract void 
>> com.github.jdnl.Main$MyLib.exit(int) : ()v
>>     at 
>> java.base/jdk.internal.foreign.NativeInvoker.of(NativeInvoker.java:127)
>>     at 
>> java.base/jdk.internal.foreign.HeaderImplGenerator.generateFunctionMethod(HeaderImplGenerator.java:130)
>>     at 
>> java.base/jdk.internal.foreign.HeaderImplGenerator.generateMethodImplementation(HeaderImplGenerator.java:121)
>>     at 
>> java.base/jdk.internal.foreign.BinderClassGenerator.generateMembers(BinderClassGenerator.java:150)
>>     ... 15 more
>> 
>> And it is thrown when binding the interface, while the original 
>> exception was thrown when invoking the method.
>> 
>> I have kept the implementation fairly permissive, so things like 
>> binding `void m(Pointer<?> p)` to `(u64)v` (converting a pointer to an 
>> integer) should still be allowed, but maybe a stricter approach is 
>> preferable? I have added a test to verify that it catches some basic 
>> mistakes like missing parameters or return types.
>> 
>> Diff: 
>> https://gist.github.com/JornVernee/2fb68d70bad1aa807b50b77840117b27
>> 
>> I saw an RFR and JBS bug was made for my last contribution (thanks 
>> Sundar!), so I've not added 'RFR' to the subject line this time.
>> 
>> Regards,
>> Jorn