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

[Maurizio Cimadamore]

I'll start the paperwork for [1]. I'll probably change it slightly to 
still use a constructor, so that there are less clashes with the ongoing 
direct invoker patch which also touches the same area. But what you did 
was sensible of course.

Regarding the other suggestions:

1) that looks sensible; right now we return a nullPointer() which as a 
References.ofVoid, so that's why you get the UOE. We could create a 
References.ofNull to be used specifically for nullPointer()

2) Yes, I'm aware of this issue; I think at this stage we don't want to 
make metadata changes - for when we will ready to rediscuss metadata 
again (soon!) I think we need to go for a more radical change here, as I 
don't think that pushing all symbol declarations on a toplevel 
annotation is helpful here. So stay tuned for more. Also, I have the 
feeling that now that the parser has improved, we could tackle this 
usability issue in another way - that is, by parsing the following grammar:

declarations ::= declaration [declarations]

declaration ::= symbolName '=' layout |
                         symbolName '=' function

symbolName ::= Ident

In the past, when I tried to do this I faced issues with the scanner 
sometimes consuming token I didn't want, but then I completely revamped 
the code for parsing annotations (and other productions too), and this 
might improve things. It might be worth another try.

Thanks
Maurizio



On 25/09/18 19:33, Jorn Vernee wrote:
> 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