[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).
Thanks
Maurizio
>
>> 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