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

Wrong calling convention used on Windows #403

Open
radekdoulik opened this issue Dec 12, 2018 · 2 comments
Open

Wrong calling convention used on Windows #403

radekdoulik opened this issue Dec 12, 2018 · 2 comments
Labels
bug Component does not function as intended java-interop Runtime bridge between .NET and Java

Comments

@radekdoulik
Copy link
Member

We should make sure we use the right calling convention on different platforms.

Recently I have run into that when trying to use Java.Runtime.Environment.dll on Windows. Turned out we don't specify right calling convention when interfacing with JVM and with own native functions as well (java_interop_gc_bridge_*).

That leads to crashes without usable stacktrace information.

@radekdoulik radekdoulik self-assigned this Dec 12, 2018
radekdoulik added a commit to radekdoulik/java.interop that referenced this issue Dec 12, 2018
Fixes (partially?) dotnet#403

Make sure we use the right calling convention by using JNICALL.

Do not force CallingConvention.Cdecl in pinvokes, as these are
different on Windows. Rather stay with the platform defaults.
radekdoulik added a commit to radekdoulik/java.interop that referenced this issue Dec 12, 2018
Fixes (partially?) dotnet#403

Make sure we use the right calling convention by using JNICALL.

Do not force CallingConvention.Cdecl in pinvokes, as these are
different on Windows. Rather stay with the platform defaults.
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 12, 2018
Context: dotnet/java-interop#403

Make sure we use `__stdcall` when crosscompiling for mxe/win.

This allows us to crosscompile for Windows without installing Java SDK
for Windows.

The include path is put BEFORE other includes, so that the `jni_md.h`
is found before the one in Mac SDK in darwin SDK subdirectory (like
/Library/Java/JavaVirtualMachines/jdk1.8.0_144.jdk/Contents/Home/include/darwin)
@jonpryor
Copy link
Member

We should make sure we use the right calling convention on different platforms.

Should we?

We need to make sure that we use the right calling convention: the C# P/Invoke declaration must match what's in the C library.

The thing is, this is already done (mostly): the default calling convention on Windows differs between C# and C. The default P/Invoke calling convention is WinAPI -- stdcall on Windows, cdecl elsewhere -- while the default calling convention for C is cdecl everywhere. This is why our C# declarations explicitly specify cdecl:

https://github.com/xamarin/java.interop/blob/b57d7703ee1af11bc16db689c6282e50d76ef121/src/Java.Runtime.Environment/Java.Interop/MonoRuntimeObjectReferenceManager.cs#L207

Not specifying CallingConvention=CallingConvention.Cdecl instead requires the use of JNICALL within our C code, meaning this:

JI_API jint
java_interop_jnienv_get_version (JNIEnv *env);

would need to become:

JI_API jint JI_CALLCONV
java_interop_jnienv_get_version (JNIEnv *env);

where JI_CALLCONV is __stdcall on Windows.

Such a change would become a significantly larger change, and I don't see any actual need for it.

All we need is for the C# and C sides to be consistent with each other. They already are.

@radekdoulik
Copy link
Member Author

I don't think we need to add the CallingConvention=CallingConvention.Cdecl as the default works OK.

These for example work without specifying Cdecl explicitly:

                [DllImport (JavaInteropLib, CharSet=CharSet.Ansi)]
                internal static extern int java_interop_jvm_load (string path);

                [DllImport (JavaInteropLib, CharSet=CharSet.Ansi)]
                internal static extern int java_interop_jvm_create (out IntPtr javavm, out IntPtr jnienv, ref JavaVMInitArgs args);

The Cdecl removal is not important part of that issue though. The wrong calling convention used to call JNI API is.

jonpryor pushed a commit to dotnet/android that referenced this issue Dec 13, 2018
Context: dotnet/java-interop#403

On Windows, JNI defines `JNICALL` as `__stdcall`: most Java APIs need
to use the `__stdcall` calling convention.  In particular this
includes the function pointers within the `JNIEnv` struct:
`JNIEnv::GetVersion()` is a `__stdcall` function pointer, as well as
most other function pointers:

	// <jni.h>
	/* partial */ struct JNINativeInterface_ {
	    jint (JNICALL *GetVersion)(JNIEnv *env);
	};

`JNICALL` is `__stdcall` on Windows, and undefined (cdecl) elsewhere.

Unfortunately, we build `src/monodroid` for Windows *from macOS* with
MXE, and we didn't have a full or complete JDK Windows installation.
The result is that we have a *calling convention mismatch* between
e.g. `libmono-android.debug.dll` and `jvm.dll`, which can result in
runtime call stack corruption.

Fix this by adding a new `jni/win32/jni_md.h` file and inserting
`jni/win32` into CMake's `include_directories` *before* the OS-native
JDK include directory, e.g.
`/Library/Java/JavaVirtualMachines/jdk1.8.0_144.jdk/Contents/Home/include/darwin`,
ensuring that `JNICALL` is set to `__stdcall` when compiling *for*
Windows *from* macOS.

This allows us to cross-compile for Windows without installing the
Java SDK for Windows.
radekdoulik added a commit to radekdoulik/java.interop that referenced this issue Dec 18, 2018
Fixes (partially?) dotnet#403

Make sure we use the right calling convention when calling JNI by
using JNICALL.

Add CallingConvention.Cdecl to few `DllImport`s to be consistent.
jonpryor pushed a commit to dotnet/android that referenced this issue Dec 18, 2018
Context: dotnet/java-interop#403

On Windows, JNI defines `JNICALL` as `__stdcall`: most Java APIs need
to use the `__stdcall` calling convention.  In particular this
includes the function pointers within the `JNIEnv` struct:
`JNIEnv::GetVersion()` is a `__stdcall` function pointer, as well as
most other function pointers:

	// <jni.h>
	/* partial */ struct JNINativeInterface_ {
	    jint (JNICALL *GetVersion)(JNIEnv *env);
	};

`JNICALL` is `__stdcall` on Windows, and undefined (cdecl) elsewhere.

Unfortunately, we build `src/monodroid` for Windows *from macOS* with
MXE, and we didn't have a full or complete JDK Windows installation.
The result is that we have a *calling convention mismatch* between
e.g. `libmono-android.debug.dll` and `jvm.dll`, which can result in
runtime call stack corruption.

Fix this by adding a new `jni/win32/jni_md.h` file and inserting
`jni/win32` into CMake's `include_directories` *before* the OS-native
JDK include directory, e.g.
`/Library/Java/JavaVirtualMachines/jdk1.8.0_144.jdk/Contents/Home/include/darwin`,
ensuring that `JNICALL` is set to `__stdcall` when compiling *for*
Windows *from* macOS.

This allows us to cross-compile for Windows without installing the
Java SDK for Windows.
jonpryor pushed a commit that referenced this issue Dec 21, 2018
Fixes? #403

Make sure we use the right calling convention when calling JNI
functions by using the `JNICALL` macro in the appropriate locations.

Set `CallingConvention=CallingConvention.Cdecl` on `[DllImport]`s
to match the calling convention of the native libraries we're using.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Jan 2, 2019
Changes: dotnet/java-interop@b57d770...0cd8bc7
Fixes?: dotnet/java-interop#403

[jcwgen] perf improvements

[build] use NuGet 4.7.1

Use right calling convention for JNI calls

Needed to fix build downstream in monodroid:

- [jnienv-gen] missing TargetFrameworkVersion
jonpryor pushed a commit to dotnet/android that referenced this issue Jan 2, 2019
Fixes? dotnet/java-interop#403

Performance improvements to `JavaCallableWrapperGenerator`;
saves ~59ms from `<GenerateJavaStubs/>` execution:

	Before
	1414 ms  GenerateJavaStubs                          1 calls

	After
	1355 ms  GenerateJavaStubs                          1 calls

Use NuGet 4.7.1 so that `nuget restore` doesn't fail when processing
`.csproj` files which contain `<Import/>`s which reference files
which do not currently exist.

Use a consistent calling convention for JNI calls.

Update `build-tools/jnienv-gen/jnienv-gen.csproj` to set
`$(TargetFrameworkVersion)` (which wasn't previously set?!) to fix
a build error observed downstream in monodroid.
@jpobst jpobst added bug Component does not function as intended java-interop Runtime bridge between .NET and Java labels Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Component does not function as intended java-interop Runtime bridge between .NET and Java
Projects
None yet
Development

No branches or pull requests

3 participants