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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Wed Sep 26 15:49:43 UTC 2018

On 26/09/18 16:34, Jorn Vernee wrote:
> 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.
Not sure you need to add a parameter to nullPointer for that. I think 
there should be only one null pointer, and we should use unchecked 
conversion internally to cast it to the type the user wants - 
dynamically the carrier will not be used - since the Reference attached 
to that will prevent dereference ops.
> 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.
The reason why carrier is hidden is that it's only part of the story - 
e.g. all pointers of pointers will have Pointer.class a carrier in the 
layout type, but some are really Pointer<Integer> some are Pointer<Byte> 
- that is a j.l.Class is not enough. And I wanted to avoid committing to 
java.lang.reflect.Type too much in this API. I get what you mean about 
asymmetry, but my feeling is that client code doesn't probably care much 
about the carrier component - the part that clients mainly care about is 
in having the carrier reflected in the generic type e.g. Pointer<X> - so 
the carrier is there, but only as a source of static safety.

Of course if you write generic code that works on any pointer Pointer<?> 
(as the binder does) then you might need to look at the carrier, but I 
wanted to wait and see what these use cases looked like first. Also, 
this is an area where pattern matching (when available) will help a lot:

Pointer<?> pointer = ...
switch (pointer.type) {
     case Pointer(Pointer(int.class)): ...

(probably similar improvements will help with layout API as well).

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

More information about the panama-dev mailing list