Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use MethodResultContext in direct mapping (as done in interface mapping) #589

Merged
merged 4 commits into from
Feb 7, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ Features
* [#577](https://github.com/java-native-access/jna/pull/577): Apply generic definitions wherever applicable - [@lgoldstein](https://github.com/lgoldstein).
* [#569](https://github.com/java-native-access/jna/pull/569): Added `com.sun.jna.platform.win32.Winspool.PRINTER_INFO_2` support. Added GetPrinter and ClosePrinter functions in `com.sun.jna.platform.win32.Winspool` - [@IvanRF](https://github.com/IvanRF).
* [#583](https://github.com/java-native-access/jna/pull/583): Added printer attributes and status - [@IvanRF](https://github.com/IvanRF).
* [#589](https://github.com/java-native-access/jna/pull/589): Use MethodResultContext in direct mapping (as done in interface mapping) - [@marco2357](https://github.com/marco2357).

Bug Fixes
---------
Expand Down
2 changes: 1 addition & 1 deletion build.xml
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@
<property name="jni.revision" value="1"/>
<property name="jni.build" value="0"/> <!--${build.number}-->
<property name="jni.version" value="${jni.major}.${jni.minor}.${jni.revision}"/>
<property name="jni.md5" value="1a6047467b59e8748f975e03016ce3d9"/>
<property name="jni.md5" value="4f72f2799dfee6008a386bc40afd7428"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to the JNI API require a bump in JNA's JNI major version, and a corresponding rebuild of all the natives.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR (bumped major version from 4 to 5).
I don't think I can rebuild the natives myself unless there's some fancy cross compilation option in JNA I don't know about. I only have Linux x64. How can I do this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll deal with the native rebuilds. I have VM setups for a number of them and access to some compile farms for the others. Build and test on whichever you have available; I’ll build/test the rest after merge.

On Feb 5, 2016, at 11:19 AM, marco2357 [email protected] wrote:

In build.xml:

@@ -72,7 +72,7 @@


Updated the PR (bumped major version from 4 to 5).
I don't think I can rebuild the natives myself unless there's some fancy cross compilation option in JNA I don't know about. I only have Linux x64. How can I do this?


Reply to this email directly or view it on GitHub.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some VMs for other platforms, so I had access to Linux and Windows, 32bit and 64bit each to do native rebuilds:

  • Linux 64bit: all good, committed a rebuild
  • Windows 32bit: all good, committed a rebuild (done with cygwin)
  • Linux 32bit: LibraryLoadTest.testLoadJAWT only worked after setting LD_LIBRARY_PATH to $JAVA_HOME/lib/i386. Otherwise all good, committed a rebuild.
  • Windows 64bit: LibraryLoadTest.testLoadJAWT and LibraryLoadTest.testLoadLibraryWithLongName fail. I couldn't get them to work. They are independent of my change but I don't feel comfortable committing the native rebuild. Maybe the problem is that I built it with cygwin instead of MSVC?

So I committed 3 natives. I'd be happy if you could take care of the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My win32 native rebuild is actually more than double the size of the previous one. Maybe you want to rebuild this yourself too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll take care of the windows rebuilds.

The long name failures have been there already, they actually expose a Sun JVM bug.

On Feb 7, 2016, at 8:56 AM, marco2357 [email protected] wrote:

In build.xml:

@@ -72,7 +72,7 @@


My win32 native rebuild is actually more than double the size of the previous one. Maybe you want to rebuild this yourself too.


Reply to this email directly or view it on GitHub.

<property name="spec.title" value="Java Native Access (JNA)"/>
<property name="spec.vendor" value="${vendor}"/>
<property name="spec.version" value="${jna.major}"/>
Expand Down
2 changes: 1 addition & 1 deletion native/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ OS=$(shell uname | sed -e 's/CYGWIN.*/win32/g' \
-e 's/Linux.*/linux/g')

JNA_JNI_VERSION=4.0.1 # auto-generated by ant
CHECKSUM=1a6047467b59e8748f975e03016ce3d9 # auto-generated by ant
CHECKSUM=4f72f2799dfee6008a386bc40afd7428 # auto-generated by ant

JAVA_INCLUDES=-I"$(JAVA_HOME)/include" \
-I"$(JAVA_HOME)/include/$(OS)"
Expand Down
2 changes: 1 addition & 1 deletion native/callback.c
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ invoke_callback(JNIEnv* env, callback *cb, ffi_cif* cif, void *resp, void **cbar
case CVT_NATIVE_MAPPED_WSTRING:
// Make sure we have space enough for the new argument
args[i+3] = alloca(sizeof(void *));
*((void **)args[i+3]) = fromNative(env, cb->arg_classes[i], cif->arg_types[i], cbargs[i], JNI_FALSE, cb->encoding);
*((void **)args[i+3]) = fromNativeCallbackParam(env, cb->arg_classes[i], cif->arg_types[i], cbargs[i], JNI_FALSE, cb->encoding);
break;
case CVT_POINTER:
*((void **)args[i+3]) = newJavaPointer(env, *(void **)cbargs[i]);
Expand Down
65 changes: 47 additions & 18 deletions native/dispatch.c
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ static jmethodID MID_DoubleBuffer_arrayOffset;

static jmethodID MID_Pointer_init;
static jmethodID MID_Native_dispose;
static jmethodID MID_Native_fromNativeCallbackParam;
static jmethodID MID_Native_fromNative;
static jmethodID MID_Native_nativeType;
static jmethodID MID_Native_toNativeTypeMapped;
Expand Down Expand Up @@ -1147,16 +1148,17 @@ static void
fromNativeTypeMapped(JNIEnv* env, jobject from_native,
void* native_return_value,
int jtype, size_t size,
jclass java_return_class,
jobject java_method,
void* result_storage,
const char* encoding) {
jobject value = new_object(env, (char)jtype, native_return_value, JNI_TRUE, encoding);
if (!(*env)->ExceptionCheck(env)) {
jobject obj = (*env)->CallStaticObjectMethod(env, classNative,
MID_Native_fromNativeTypeMapped,
from_native, value, java_return_class);
from_native, value, java_method);
if (!(*env)->ExceptionCheck(env)) {
// Convert objects into primitive types if the return class demands it
jclass java_return_class = (*env)->CallObjectMethod(env, java_method, MID_Method_getReturnType);
if ((*env)->IsSameObject(env, java_return_class, classPrimitiveBoolean)
|| (*env)->IsSameObject(env, java_return_class, classPrimitiveByte)
|| (*env)->IsSameObject(env, java_return_class, classPrimitiveCharacter)
Expand All @@ -1175,17 +1177,29 @@ fromNativeTypeMapped(JNIEnv* env, jobject from_native,
}

jobject
fromNative(JNIEnv* env, jclass javaClass, ffi_type* type, void* resp, jboolean promote, const char* encoding) {
fromNativeCallbackParam(JNIEnv* env, jclass javaClass, ffi_type* type, void* resp, jboolean promote, const char* encoding) {
int jtype = get_java_type_from_ffi_type(type);
jobject value = new_object(env, (char)jtype, resp, promote, encoding);
if (!(*env)->ExceptionCheck(env)) {
return (*env)->CallStaticObjectMethod(env, classNative,
MID_Native_fromNative,
MID_Native_fromNativeCallbackParam,
javaClass, value);
}
return NULL;
}

jobject
fromNative(JNIEnv* env, jobject javaMethod, ffi_type* type, void* resp, jboolean promote, const char* encoding) {
int jtype = get_java_type_from_ffi_type(type);
jobject value = new_object(env, (char)jtype, resp, promote, encoding);
if (!(*env)->ExceptionCheck(env)) {
return (*env)->CallStaticObjectMethod(env, classNative,
MID_Native_fromNative,
javaMethod, value);
}
return NULL;
}


static ffi_type*
getStructureType(JNIEnv *env, jobject obj) {
Expand Down Expand Up @@ -1670,7 +1684,7 @@ typedef struct _method_data {
ffi_type** closure_arg_types;
int* flags;
int rflag;
jclass closure_rclass;
jobject closure_method;
jobject* to_native;
jobject from_native;
jboolean throw_last_error;
Expand Down Expand Up @@ -1860,15 +1874,15 @@ dispatch_direct(ffi_cif* cif, void* volatile resp, void** argp, void *cdata) {
? 'c' : (data->rflag == CVT_TYPE_MAPPER_WSTRING
? 'w' : get_java_type_from_ffi_type(data->cif.rtype)));
fromNativeTypeMapped(env, data->from_native, resp, jtype, data->cif.rtype->size,
data->closure_rclass, oldresp, data->encoding);
data->closure_method, oldresp, data->encoding);
}
break;
case CVT_INTEGER_TYPE:
case CVT_POINTER_TYPE:
case CVT_NATIVE_MAPPED:
case CVT_NATIVE_MAPPED_STRING:
case CVT_NATIVE_MAPPED_WSTRING:
*(void **)oldresp = fromNative(env, data->closure_rclass, data->cif.rtype, resp, JNI_TRUE, data->encoding);
*(void **)oldresp = fromNative(env, data->closure_method, data->cif.rtype, resp, JNI_TRUE, data->encoding);
break;
case CVT_POINTER:
*(void **)resp = newJavaPointer(env, *(void **)resp);
Expand All @@ -1880,13 +1894,22 @@ dispatch_direct(ffi_cif* cif, void* volatile resp, void** argp, void *cdata) {
*(void **)resp = newJavaWString(env, *(void **)resp);
break;
case CVT_STRUCTURE:
*(void **)resp = newJavaStructure(env, *(void **)resp, data->closure_rclass);
{
jclass return_class = (*env)->CallObjectMethod(env, data->closure_method, MID_Method_getReturnType);
*(void **)resp = newJavaStructure(env, *(void **)resp, return_class);
}
break;
case CVT_STRUCTURE_BYVAL:
*(void **)oldresp = newJavaStructure(env, resp, data->closure_rclass);
{
jclass return_class = (*env)->CallObjectMethod(env, data->closure_method, MID_Method_getReturnType);
*(void **)oldresp = newJavaStructure(env, resp, return_class);
}
break;
case CVT_CALLBACK:
*(void **)resp = newJavaCallback(env, *(void **)resp, data->closure_rclass);
{
jclass return_class = (*env)->CallObjectMethod(env, data->closure_method, MID_Method_getReturnType);
*(void **)resp = newJavaCallback(env, *(void **)resp, return_class);
}
break;
default:
break;
Expand Down Expand Up @@ -2749,11 +2772,17 @@ Java_com_sun_jna_Native_initIDs(JNIEnv *env, jclass cls) {
throwByName(env, EUnsatisfiedLink,
"Can't obtain static method dispose from class com.sun.jna.Native");
}
else if (!(MID_Native_fromNative
else if (!(MID_Native_fromNativeCallbackParam
= (*env)->GetStaticMethodID(env, classNative,
"fromNative", "(Ljava/lang/Class;Ljava/lang/Object;)Lcom/sun/jna/NativeMapped;"))) {
throwByName(env, EUnsatisfiedLink,
"Can't obtain static method fromNative from class com.sun.jna.Native");
"Can't obtain static method fromNative(Class, Object) from class com.sun.jna.Native");
}
else if (!(MID_Native_fromNative
= (*env)->GetStaticMethodID(env, classNative,
"fromNative", "(Ljava/lang/reflect/Method;Ljava/lang/Object;)Lcom/sun/jna/NativeMapped;"))) {
throwByName(env, EUnsatisfiedLink,
"Can't obtain static method fromNative(Method, Object) from class com.sun.jna.Native");
}
else if (!(MID_Native_nativeType
= (*env)->GetStaticMethodID(env, classNative,
Expand All @@ -2769,9 +2798,9 @@ Java_com_sun_jna_Native_initIDs(JNIEnv *env, jclass cls) {
}
else if (!(MID_Native_fromNativeTypeMapped
= (*env)->GetStaticMethodID(env, classNative,
"fromNative", "(Lcom/sun/jna/FromNativeConverter;Ljava/lang/Object;Ljava/lang/Class;)Ljava/lang/Object;"))) {
"fromNative", "(Lcom/sun/jna/FromNativeConverter;Ljava/lang/Object;Ljava/lang/reflect/Method;)Ljava/lang/Object;"))) {
throwByName(env, EUnsatisfiedLink,
"Can't obtain static method fromNative from class com.sun.jna.Native");
"Can't obtain static method fromNative(FromNativeConverter, Object, Method) from class com.sun.jna.Native");
}
else if (!LOAD_CREF(env, Structure, "com/sun/jna/Structure")) {
throwByName(env, EUnsatisfiedLink,
Expand Down Expand Up @@ -3250,7 +3279,7 @@ Java_com_sun_jna_Native_unregister(JNIEnv *env, jclass UNUSED(ncls), jclass cls,
}
}
if (md->from_native) (*env)->DeleteWeakGlobalRef(env, md->from_native);
if (md->closure_rclass) (*env)->DeleteWeakGlobalRef(env, md->closure_rclass);
if (md->closure_method) (*env)->DeleteGlobalRef(env, md->closure_method);
free(md->arg_types);
free(md->closure_arg_types);
free(md->flags);
Expand All @@ -3277,7 +3306,7 @@ Java_com_sun_jna_Native_registerMethod(JNIEnv *env, jclass UNUSED(ncls),
jint rconversion,
jlong closure_return_type,
jlong return_type,
jclass closure_rclass,
jobject closure_method,
jlong function, jint cc,
jboolean throw_last_error,
jobjectArray to_native,
Expand Down Expand Up @@ -3315,7 +3344,7 @@ Java_com_sun_jna_Native_registerMethod(JNIEnv *env, jclass UNUSED(ncls),
data->closure_arg_types = malloc(sizeof(ffi_type*) * (argc + 2));
data->closure_arg_types[0] = &ffi_type_pointer;
data->closure_arg_types[1] = &ffi_type_pointer;
data->closure_rclass = NULL;
data->closure_method = NULL;
data->flags = cvts ? malloc(sizeof(jint)*argc) : NULL;
data->rflag = rconversion;
data->to_native = NULL;
Expand All @@ -3342,7 +3371,7 @@ Java_com_sun_jna_Native_registerMethod(JNIEnv *env, jclass UNUSED(ncls),
if (closure_types) (*env)->ReleaseLongArrayElements(env, closure_atypes, closure_types, 0);
if (cvts) (*env)->ReleaseIntArrayElements(env, conversions, cvts, 0);
data->fptr = L2A(function);
data->closure_rclass = (*env)->NewWeakGlobalRef(env, closure_rclass);
data->closure_method = (*env)->NewGlobalRef(env, closure_method);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a global here and not a weak global? The method object should stay around as long as the callback class is extant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised it worked before with a weak global.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it not work for you with a weak global? The return type class will be referenced by the callback class, although I'm not sure how the JVM manages references to Method objects; it's quite possible Class objects don't actually have any references to Method objects and they're only created on demand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At any rate, if you keep a hard reference on the JNA side, you need to ensure you explicitly release it when the closure data is disposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the weak global is collected sooner or later. The testsuite fails. Took me a while to figure out why (I assumed since a weak global was enough for the Class it should be enough for the Method).
It's not enough to do it in Java_com_sun_jna_Native_unregister() as it is now? Where else does it have to be released?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing it in unregister is probably sufficient.

On Feb 5, 2016, at 11:23 AM, marco2357 [email protected] wrote:

In native/dispatch.c:

@@ -3342,7 +3371,7 @@ Java_com_sun_jna_Native_registerMethod(JNIEnv _env, jclass UNUSED(ncls),
if (closure_types) (_env)->ReleaseLongArrayElements(env, closure_atypes, closure_types, 0);
if (cvts) (*env)->ReleaseIntArrayElements(env, conversions, cvts, 0);
data->fptr = L2A(function);

  • data->closure_rclass = (*env)->NewWeakGlobalRef(env, closure_rclass);
  • data->closure_method = (*env)->NewGlobalRef(env, closure_method);

It's not enough to do it in Java_com_sun_jna_Native_unregister() as it is now? Where else does it have to be released?


Reply to this email directly or view it on GitHub.


status = ffi_prep_cif(closure_cif, abi, argc+2, closure_rtype, data->closure_arg_types);
if (ffi_error(env, "Native method mapping", status)) {
Expand Down
2 changes: 1 addition & 1 deletion native/dispatch.h
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ extern void* getPointerTypeAddress(JNIEnv*, jobject);
extern void writeStructure(JNIEnv*, jobject);
extern jclass getNativeType(JNIEnv*, jclass);
extern void toNative(JNIEnv*, jobject, void*, size_t, jboolean, const char*);
extern jclass fromNative(JNIEnv*, jclass, ffi_type*, void*, jboolean, const char*);
extern jclass fromNativeCallbackParam(JNIEnv*, jclass, ffi_type*, void*, jboolean, const char*);

typedef struct _AttachOptions {
int daemon;
Expand Down
17 changes: 10 additions & 7 deletions src/com/sun/jna/Native.java
Original file line number Diff line number Diff line change
Expand Up @@ -1716,7 +1716,7 @@ public static void register(Class<?> cls, NativeLibrary lib) {
sig, cvt,
closure_atypes, atypes, rcvt,
closure_rtype, rtype,
rclass,
method,
f.peer, f.getCallingConvention(),
throwLastError,
toNative, fromNative,
Expand Down Expand Up @@ -1769,7 +1769,7 @@ private static native long registerMethod(Class<?> cls,
int rconversion,
long closure_rtype,
long rtype,
Class<?> rclass,
Method method,
long fptr,
int callingConvention,
boolean throwLastError,
Expand All @@ -1780,11 +1780,15 @@ private static native long registerMethod(Class<?> cls,

// Called from native code
private static NativeMapped fromNative(Class<?> cls, Object value) {
// NOTE: technically should be either CallbackParameterContext or
// FunctionResultContext
// NOTE: technically should be CallbackParameterContext
return (NativeMapped)NativeMappedConverter.getInstance(cls).fromNative(value, new FromNativeContext(cls));
}
// Called from native code
private static NativeMapped fromNative(Method m, Object value) {
Class<?> cls = m.getReturnType();
return (NativeMapped)NativeMappedConverter.getInstance(cls).fromNative(value, new MethodResultContext(cls, null, null, m));
}
// Called from native code
private static Class<?> nativeType(Class<?> cls) {
return NativeMappedConverter.getInstance(cls).nativeType();
}
Expand All @@ -1795,9 +1799,8 @@ private static Object toNative(ToNativeConverter cvt, Object o) {
return cvt.toNative(o, new ToNativeContext());
}
// Called from native code
private static Object fromNative(FromNativeConverter cvt, Object o, Class<?> cls) {
// NOTE: technically should be FunctionResultContext
return cvt.fromNative(o, new FromNativeContext(cls));
private static Object fromNative(FromNativeConverter cvt, Object o, Method m) {
return cvt.fromNative(o, new MethodResultContext(m.getReturnType(), null, null, m));
}

/** Create a new cif structure. */
Expand Down
47 changes: 47 additions & 0 deletions test/com/sun/jna/DirectTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -282,5 +282,52 @@ public String getFunctionName(NativeLibrary lib, Method method) {
fail("Native method was not properly mapped: " + e);
}
}

public static class PointerNativeMapped implements NativeMapped {
String nativeMethodName;
@Override
public PointerNativeMapped fromNative(Object nativeValue, FromNativeContext context) {
nativeMethodName = ((MethodResultContext)context).getMethod().getName();
return this;
}
@Override
public Object toNative() {
return null;
}
@Override
public Class<?> nativeType() {
return Pointer.class;
}
}
public static class PointerTypeMapped {
String nativeMethodName;
}
public static class FromNativeTests {
static native PointerNativeMapped returnPointerArgument(PointerNativeMapped arg);
static native PointerTypeMapped returnPointerArgument(PointerTypeMapped arg);
}
public void testDirectMappingFromNative() {
DefaultTypeMapper mapper = new DefaultTypeMapper();
mapper.addTypeConverter(PointerTypeMapped.class, new TypeConverter() {
@Override
public PointerTypeMapped fromNative(Object nativeValue, FromNativeContext context) {
PointerTypeMapped ret = new PointerTypeMapped();
ret.nativeMethodName = ((MethodResultContext)context).getMethod().getName();
return ret;
}
@Override
public Object toNative(Object value, ToNativeContext context) {
return null;
}
@Override
public Class<?> nativeType() {
return Pointer.class;
}
});
NativeLibrary lib = NativeLibrary.getInstance("testlib", Collections.singletonMap(Library.OPTION_TYPE_MAPPER, mapper));
Native.register(FromNativeTests.class, lib);
assertEquals("Failed to access MethodResultContext", "returnPointerArgument", FromNativeTests.returnPointerArgument(new PointerNativeMapped()).nativeMethodName);
assertEquals("Failed to access MethodResultContext", "returnPointerArgument", FromNativeTests.returnPointerArgument(new PointerTypeMapped()).nativeMethodName);
}
}