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

Provide proper calling conversion for x86 framework #1833

Merged
merged 7 commits into from Dec 7, 2018

Conversation

Projects
None yet
5 participants
@Ivanidzo4ka
Member

Ivanidzo4ka commented Dec 6, 2018

Fixes #1721
Right now if we call our nuget from .net framework 4.6 x86, we get ugly PInvokeStackImbalance exception.

I've check and it looks like we build our native libraries in __csdecl conversion which is standard for C and we have
#define EXPORT_API(ret) extern "C" __declspec(dllexport) ret in our stdafx.h file.
by default PInvoke use WinApi convention which falls down to _stdcall.

On x64 platforms and .net core everything works fine, I assume as part of implementation.
For .net framework 4.6 with x86, it throws exception.

So I put implicit calling conversion in all our PInvoke calls, and also made sure we use our stdafx.h in all our native libraries.

Have no idea how to write test for this tho.

@Ivanidzo4ka Ivanidzo4ka requested review from wschin, eerhardt, codemzs and TomFinley Dec 6, 2018

@tannergooding

This comment has been minimized.

Member

tannergooding commented Dec 6, 2018

To my knowledge:

  • On x86, Windows defaults to WinApi (which is StdCall); while Linux defaults to Cdecl
  • On x64, the same defaults apply but on Windows all the calling conventions were folded together into the single Windows x64 calling convention

@jkotas may know if there is something else that changed on .NET Core that would allow x86 to pass without explicitly specifying the correct calling convention

  • I would have expected this to fail, given that x86 has actually diferences and who cleans the stack changes
@jkotas

This comment has been minimized.

Member

jkotas commented Dec 6, 2018

I've check and it looks like we build our native libraries in __csdecl conversion which is standard for C
#define EXPORT_API(ret) extern "C" __declspec(dllexport) ret i

On Windows, the standard calling convention to use is configurable with compiler command line switches. The de-facto standard is to use stdcall as default on Windows x86. You need to actually look at the compiler switches to figure out which calling convention this is using.

@jkotas

This comment has been minimized.

Member

jkotas commented Dec 6, 2018

I would have expected this to fail, given that x86 has actually difference

If this used to work on x86 before open sourcing, I think that the most likely cause was that the compiler switches got messed up by port to CMake.

My recommendation would be to avoid explicitly specified calling conventions in C# as much as possible, and just configure the compiler to use the right one as the default.

@eerhardt

This comment has been minimized.

Member

eerhardt commented Dec 6, 2018

My recommendation would be to avoid explicitly specified calling conventions in C# as much as possible,

I agree.

and just configure the compiler to use the right one as the default.

I was thinking we should just follow the same pattern as we do for clrcompression.dll in corefx:

https://github.com/dotnet/corefx/blob/8f2afd786b04ba95b13d5a50b6f1e566b16cf5ac/src/Native/AnyOS/zlib/pal_zlib.h#L5-L15

Would that work as well?

@jkotas

This comment has been minimized.

Member

jkotas commented Dec 6, 2018

Would that work as well?

I do not see anything particularly wrong with the macros that you have in this repo right now. The key piece that clrcompression has, but this repo does not, is the /Gz command line switch for Windows x86: https://github.com/dotnet/corefx/blob/8f2afd786b04ba95b13d5a50b6f1e566b16cf5ac/src/Native/Windows/CMakeLists.txt#L48

@eerhardt

This comment has been minimized.

Member

eerhardt commented Dec 6, 2018

If this used to work on x86 before open sourcing, I think that the most likely cause was that the compiler switches got messed up by port to CMake.

Digging into the internal repo, they use checked in .vcxproj files. And looking at the project properties:

image

@Ivanidzo4ka

This comment has been minimized.

Member

Ivanidzo4ka commented Dec 6, 2018

I'll try to add add_compile_options(/Gz) and remove any calling conventions from our PInvokes.

Where is one thing which blocking me right now - SymSGD with it call to Mkl
SymSgdNative.obj : error LNK2019: unresolved external symbol _cblas_sdot@20 referenced in function "float __stdcall SDOT(int,float const *,float const *)" (?SDOT@@YGMHPBM0@Z) [D:\src\fork-machinelearning\bin\obj\x86.Debug\Native\SymSgdNative\SymSgdNative.vcxproj] [d:\src\fork-machinelearning\src\Native\build.proj]

Which I think related to Mkl library been build with __cdecl.

@eerhardt

This comment has been minimized.

Member

eerhardt commented Dec 6, 2018

Which I think related to Mkl library been build with __cdecl.

We extern those function calls:

extern "C" float cblas_sdot(const int vecSize, const float* denseVecX, const int incX, const float* denseVecY, const int incY);
extern "C" float cblas_sdoti(const int sparseVecSize, const float* sparseVecValues, const int* sparseVecIndices, float* denseVec);
extern "C" void cblas_saxpy(const int vecSize, const float coef, const float* denseVecX, const int incX, float* denseVecY, const int incY);
extern "C" void cblas_saxpyi(const int sparseVecSize, const float coef, const float* sparseVecValues, const int* sparseVecIndices, float* denseVec);

We can just explicitly add cdecl on those extern functions.

Ivanidzo4ka added some commits Dec 6, 2018

Revert cdecl everywhere.
Put cdecl for MKL invokes.
Compile win32 with /Gz option (stdcall)
@@ -285,7 +285,7 @@ private static int CbitHighZero(ulong u)
/// returning the quotient and placing the remainder in <paramref name="rem"/>. Throws on overflow.
/// </summary>
#if !CORECLR
[DllImport(Thunk.NativePath), SuppressUnmanagedCodeSecurity]
[DllImport(Thunk.NativePath, CallingConvention =CallingConvention.Cdecl), SuppressUnmanagedCodeSecurity]

This comment has been minimized.

@wschin

wschin Dec 6, 2018

Member
Suggested change Beta
[DllImport(Thunk.NativePath, CallingConvention =CallingConvention.Cdecl), SuppressUnmanagedCodeSecurity]
[DllImport(Thunk.NativePath, CallingConvention = CallingConvention.Cdecl), SuppressUnmanagedCodeSecurity]
``` #Resolved

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 6, 2018

Member

I reverted this change


In reply to: 239656605 [](ancestors = 239656605)

@@ -70,13 +70,14 @@ public override IntArray[] Split(int[][] assignment)
}
#if USE_FASTTREENATIVE
[DllImport("FastTreeNative", CallingConvention = CallingConvention.StdCall)]
internal const string NativePath = "FastTreeNative";
[DllImport(NativePath)]
private static extern unsafe int C_Sumup_float(

This comment has been minimized.

@wschin

wschin Dec 6, 2018

Member

Do you need CallingConvention = CallingConvention.Cdecl here? The same question to private static extern unsafe int C_Sumup_double. #Resolved

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 7, 2018

Member

No, everything except MKL should work via default calls.
MKL should use cdecl.


In reply to: 239656885 [](ancestors = 239656885)

@@ -1090,7 +1090,7 @@ private static void PermutationSort(int[] permutation, double[] scores, short[]
}));
}
[DllImport("FastTreeNative", EntryPoint = "C_GetDerivatives", CallingConvention = CallingConvention.StdCall, CharSet = CharSet.Ansi)]
[DllImport("FastTreeNative", EntryPoint = "C_GetDerivatives", CallingConvention = CallingConvention.Cdecl, CharSet = CharSet.Ansi)]

This comment has been minimized.

@wschin

wschin Dec 6, 2018

Member

Some functions use StdCall and some functions use Cdecl for importing things from FastTreeNative. Do we have different calling conventions in a single DLL? #Resolved

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 7, 2018

Member

Not any more!


In reply to: 239658091 [](ancestors = 239658091)

@@ -833,7 +833,7 @@ private static unsafe class Native
}
}
[DllImport(DllName), SuppressUnmanagedCodeSecurity]
[DllImport(NativePath, CallingConvention = CallingConvention.Cdecl), SuppressUnmanagedCodeSecurity]

This comment has been minimized.

@wschin

wschin Dec 6, 2018

Member

This is the only Cdecl in this C# file? #Resolved

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 7, 2018

Member

not any more!


In reply to: 239658431 [](ancestors = 239658431)

private static extern int FreeDescriptor(ref IntPtr desc);
// See: https://software.intel.com/en-us/node/521981
[DllImport(DllProxyName, EntryPoint = "MKLDftiSetValue", CallingConvention = CallingConvention.Cdecl, CharSet = CharSet.Auto)]
[DllImport(DllProxyName, EntryPoint = "MKLDftiSetValue", CharSet = CharSet.Auto)]

This comment has been minimized.

@wschin

wschin Dec 7, 2018

Member

It looks we are using MKL. Is it not a Cdecl? #Resolved

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 7, 2018

Member

this is a proxy which we build ourselves, so it would use default convention. We can't change Mkl (which is not proxy) so for pure Mkl we have to use cdecl.


In reply to: 239658702 [](ancestors = 239658702)

add_subdirectory(FastTreeNative)
add_subdirectory(LdaNative)
add_subdirectory(MatrixFactorizationNative)
add_subdirectory(FactorizationMachineNative)

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 7, 2018

Member

revert #Resolved

[DllImport(DllPath), SuppressUnmanagedCodeSecurity]
[DllImport(NativePath), SuppressUnmanagedCodeSecurity]

This comment has been minimized.

@wschin

wschin Dec 7, 2018

Member

Would it be Cdecl? I saw EXPORT_API(void) MFDestroyModel(mf_model *&model); in machinelearning\src\Native\MatrixFactorizationNative\UnmanagedMemory.h without extra stdcall. On the other hand, stdcall looks like the default calling convention in C# files. #ByDesign

This comment has been minimized.

@Ivanidzo4ka

Ivanidzo4ka Dec 7, 2018

Member

it all depend on you compiler.
for x64 on windows it's stdcall.
I added compiler option to build in stdcall for x86.
Linux would use cdecl.

Basically as long as we use same definition for EXPORT_API we should use same default calling convention.


In reply to: 239662401 [](ancestors = 239662401)

@wschin

wschin approved these changes Dec 7, 2018

@@ -70,13 +71,14 @@ public override IntArray[] Split(int[][] assignment)
}
#if USE_FASTTREENATIVE
[DllImport("FastTreeNative", CallingConvention = CallingConvention.StdCall)]
internal const string NativePath = "FastTreeNative";
[DllImport(NativePath), SuppressUnmanagedCodeSecurity]

This comment has been minimized.

@eerhardt

eerhardt Dec 7, 2018

Member

@jkotas - is the SuppressUnmanagedCodeSecurity attribute necessary/useful on either .NET Core or .NET Framework 4.6.1+? I'm wondering if we should just get rid of it in our code.

This comment has been minimized.

@jkotas

jkotas Dec 7, 2018

Member

It is useless for .NET Core. It is needed for good perf on .NET Framework, unfortunately.

@@ -32,6 +32,7 @@ if(WIN32)
add_compile_options(/FC) # use full pathnames in diagnostics
add_compile_options(/DEBUG)
add_compile_options(/GS)
add_compile_options(/Gz)

This comment has been minimized.

@eerhardt

Just a couple minor questions, but other than those looks really good.

@Ivanidzo4ka Ivanidzo4ka merged commit 2c87b19 into dotnet:master Dec 7, 2018

2 checks passed

MachineLearning-CI #20181207.2 succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment