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

Wed Sep 26 15:34:22 UTC 2018

Maurizio Cimadamore schreef op 2018-09-26 16:42:
> 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()

Heh, I was just working on a patch which does exactly that :)

I also made it so that `nullPointer()` takes a LayoutType, so that a 
method returning e.g. `Pointer<Integer>` doesn't suddenly have a 
LayoutType with a Void carrier. Seeing the References.ofVoid anonymous 
class in the stack trace was also a source of confusion for me.

Also, a lot of places do casts to get at the carrier type of 
LayoutTypeImpl, and I've had to do that as well. I'm wondering if 
`Class<X> carrier()` should be a public method on LayoutType? Since a 
LayoutType seems to capture a mapping between a Java carrier type and a 
Layout, only being able to access the Layout through the public API 
feels a-symmetrical to me.

> 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.

Cool! I'll stay tuned.

Jorn

> 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