From ee05c8a322b3083f147d2c2d1464e381fdd19756 Mon Sep 17 00:00:00 2001 From: Johan Lorensson Date: Wed, 6 Jul 2016 06:32:08 +0200 Subject: [PATCH] Multiple fixes for Windows x86 p/invoke test failures. (#3186) * Fix for non GCC win x86 calling convention when passing empty structs over pinvoke. Port of https://github.com/mono/mono/pull/3085 to Winwdows x86 fixing failures in pinvoke2 tests: test_0_marshal_empty_struct * Fix for incorrect implementation of small struct (single float/double) on Windows x86. Failure in pinvoke3 test_0_marshal_small_struct_delegate9 is caused by incorrect implemented small struct ABI on Windows x86. Windows x86 ABI for returning structs of size 4 or 8 bytes (regardless of type) dictates that values are passed in EDX:EAX register pairs, https://msdn.microsoft.com/en-us/library/984x0h58.aspx. This is different compared to for example float or double return types (not in struct) that will be returned in ST(0), https://msdn.microsoft.com/en-us/library/ha59cbfz.aspx. Fix disables the alternative to do special handling of structs with 1 float or double and make sure same logic is always applied to all small structs regardless type as dictated by Windows x86 ABI. * mono_test_marshal_delegate_ref_delegate uses incorrect calling convention. mono_test_marshal_delegate_ref_delegate called by pinvoke3::test_55_marshal_delegate_ref_delegate uses incorrect calling convention. Mono assumes delegates to be stdcall but current definitions will be cdecl, this will cause an incorrect stack pointer on architectures where cdecl and stdcall are different (Windows x86). Test fails with "corrupt" stack pointer on Windows x86. * Smaller adjustment based on feedback. * Changed boolean struct member to bit field (on both x86 and x64) * Changed compare code style from 0 == x to x == 0 to comply with coding standard. --- mono/mini/mini-amd64.h | 2 +- mono/mini/mini-x86.c | 53 +++++++++++++++++++++++++++++++++++------- mono/mini/mini-x86.h | 1 + mono/tests/libtest.c | 6 ++--- 4 files changed, 50 insertions(+), 12 deletions(-) diff --git a/mono/mini/mini-amd64.h b/mono/mini/mini-amd64.h index 11fc30a435f..fb65862bbc1 100644 --- a/mono/mini/mini-amd64.h +++ b/mono/mini/mini-amd64.h @@ -269,7 +269,7 @@ typedef struct { int nregs; /* Only if storage == ArgOnStack */ int arg_size; // Bytes, will always be rounded up/aligned to 8 byte boundary - gboolean pass_empty_struct; // Set in scenarios when empty structs needs to be represented as argument. + guint8 pass_empty_struct : 1; // Set in scenarios when empty structs needs to be represented as argument. } ArgInfo; typedef struct { diff --git a/mono/mini/mini-x86.c b/mono/mini/mini-x86.c index badf12b33b3..8fc2c8b5423 100644 --- a/mono/mini/mini-x86.c +++ b/mono/mini/mini-x86.c @@ -231,19 +231,49 @@ add_valuetype (MonoMethodSignature *sig, ArgInfo *ainfo, MonoType *type, klass = mono_class_from_mono_type (type); size = mini_type_stack_size_full (&klass->byval_arg, NULL, sig->pinvoke); +#if defined(TARGET_WIN32) + /* + * Standard C and C++ doesn't allow empty structs, empty structs will always have a size of 1 byte. + * GCC have an extension to allow empty structs, https://gcc.gnu.org/onlinedocs/gcc/Empty-Structures.html. + * This cause a little dilemma since runtime build using none GCC compiler will not be compatible with + * GCC build C libraries and the other way around. On platforms where empty structs has size of 1 byte + * it must be represented in call and cannot be dropped. + */ + if (size == 0 && MONO_TYPE_ISSTRUCT (type) && sig->pinvoke) { + /* Empty structs (1 byte size) needs to be represented in a stack slot */ + ainfo->pass_empty_struct = TRUE; + size = 1; + } +#endif + #ifdef SMALL_STRUCTS_IN_REGS if (sig->pinvoke && is_return) { MonoMarshalType *info; - /* - * the exact rules are not very well documented, the code below seems to work with the - * code generated by gcc 3.3.3 -mno-cygwin. - */ info = mono_marshal_load_type_info (klass); g_assert (info); ainfo->pair_storage [0] = ainfo->pair_storage [1] = ArgNone; + /* Ignore empty struct return value, if used. */ + if (info->num_fields == 0 && ainfo->pass_empty_struct) { + ainfo->storage = ArgValuetypeInReg; + return; + } + + /* + * Windows x86 ABI for returning structs of size 4 or 8 bytes (regardless of type) dictates that + * values are passed in EDX:EAX register pairs, https://msdn.microsoft.com/en-us/library/984x0h58.aspx. + * This is different compared to for example float or double return types (not in struct) that will be returned + * in ST(0), https://msdn.microsoft.com/en-us/library/ha59cbfz.aspx. + * + * Apples OSX x86 ABI for returning structs of size 4 or 8 bytes uses a slightly different approach. + * If a struct includes only one scalar value, it will be handled with the same rules as scalar values. + * This means that structs with one float or double will be returned in ST(0). For more details, + * https://developer.apple.com/library/mac/documentation/DeveloperTools/Conceptual/LowLevelABI/130-IA-32_Function_Calling_Conventions/IA32.html. + */ +#if !defined(TARGET_WIN32) + /* Special case structs with only a float member */ if (info->num_fields == 1) { int ftype = mini_get_underlying_type (info->fields [0].field->type)->type; @@ -258,6 +288,8 @@ add_valuetype (MonoMethodSignature *sig, ArgInfo *ainfo, MonoType *type, return; } } +#endif + if ((info->native_size == 1) || (info->native_size == 2) || (info->native_size == 4) || (info->native_size == 8)) { ainfo->storage = ArgValuetypeInReg; ainfo->pair_storage [0] = ArgInIReg; @@ -292,7 +324,7 @@ add_valuetype (MonoMethodSignature *sig, ArgInfo *ainfo, MonoType *type, * For x86 ELF, see the "System V Application Binary Interface Intel386 * Architecture Processor Supplment, Fourth Edition" document for more * information. - * For x86 win32, see ???. + * For x86 win32, see https://msdn.microsoft.com/en-us/library/984x0h58.aspx. */ static CallInfo* get_call_info_internal (CallInfo *cinfo, MonoMethodSignature *sig) @@ -1355,7 +1387,7 @@ mono_arch_emit_call (MonoCompile *cfg, MonoCallInst *call) sentinelpos = sig->sentinelpos + (sig->hasthis ? 1 : 0); if (sig_ret && MONO_TYPE_ISSTRUCT (sig_ret)) { - if (cinfo->ret.storage == ArgValuetypeInReg) { + if (cinfo->ret.storage == ArgValuetypeInReg && cinfo->ret.pair_storage[0] != ArgNone ) { /* * Tell the JIT to use a more efficient calling convention: call using * OP_CALL, compute the result location after the call, and save the @@ -1437,7 +1469,7 @@ mono_arch_emit_call (MonoCompile *cfg, MonoCallInst *call) size = mini_type_stack_size_full (&in->klass->byval_arg, &align, sig->pinvoke); } - if (size > 0) { + if (size > 0 || ainfo->pass_empty_struct) { arg->opcode = OP_OUTARG_VT; arg->sreg1 = in->dreg; arg->klass = in->klass; @@ -1568,7 +1600,12 @@ mono_arch_emit_outarg_vt (MonoCompile *cfg, MonoInst *ins, MonoInst *src) MONO_EMIT_NEW_STORE_MEMBASE (cfg, OP_STORE_MEMBASE_REG, X86_ESP, ainfo->offset, src->dreg); } else if (size <= 4) { int dreg = mono_alloc_ireg (cfg); - MONO_EMIT_NEW_LOAD_MEMBASE (cfg, dreg, src->dreg, 0); + if (ainfo->pass_empty_struct) { + //Pass empty struct value as 0 on platforms representing empty structs as 1 byte. + MONO_EMIT_NEW_ICONST (cfg, dreg, 0); + } else { + MONO_EMIT_NEW_LOAD_MEMBASE (cfg, dreg, src->dreg, 0); + } MONO_EMIT_NEW_STORE_MEMBASE (cfg, OP_STORE_MEMBASE_REG, X86_ESP, ainfo->offset, dreg); } else if (size <= 20) { mini_emit_memcpy (cfg, X86_ESP, ainfo->offset, src->dreg, 0, size, 4); diff --git a/mono/mini/mini-x86.h b/mono/mini/mini-x86.h index 4150cdf60d0..b70d836c2aa 100644 --- a/mono/mini/mini-x86.h +++ b/mono/mini/mini-x86.h @@ -315,6 +315,7 @@ typedef struct { /* Only if storage == ArgValuetypeInReg */ ArgStorage pair_storage [2]; gint8 pair_regs [2]; + guint8 pass_empty_struct : 1; // Set in scenarios when empty structs needs to be represented as argument. } ArgInfo; typedef struct { diff --git a/mono/tests/libtest.c b/mono/tests/libtest.c index 1f2930da965..8f1cef273f4 100644 --- a/mono/tests/libtest.c +++ b/mono/tests/libtest.c @@ -889,12 +889,12 @@ mono_test_marshal_return_delegate (SimpleDelegate delegate) return delegate; } -typedef int DelegateByrefDelegate (void *); +typedef int (STDCALL *DelegateByrefDelegate) (void *); LIBTEST_API int STDCALL mono_test_marshal_delegate_ref_delegate (DelegateByrefDelegate del) { - int (*ptr) (int i); + int (STDCALL *ptr) (int i); del (&ptr); @@ -1243,7 +1243,7 @@ mono_test_marshal_stringbuilder_ref (char **s) * C/C++ standard and the runtime. */ typedef struct { -#if !defined(__GNUC__) || (defined(TARGET_WIN32) && defined(TARGET_AMD64)) +#if !defined(__GNUC__) || defined(TARGET_WIN32) char a; #endif } EmptyStruct; -- 2.25.1