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.

diff --git a/mono/mini/mini-amd64.h b/mono/mini/mini-amd64.h
index 11fc30a435f28b08366bd6cae009fe21602147d4..fb65862bbc19b15285cb7a10c20392c86a287b9a 100644 (file)
--- 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 badf12b33b3bf1d72f1466b48e8c8a0aeb517e88..8fc2c8b54236716d92170f1159820ade47d33718 100644 (file)
--- 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 4150cdf60d0aed1d2527748e11d940407bec8b7a..b70d836c2aa07fd9758bf632c62cb5cd404582a1 100644 (file)
--- 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 1f2930da9655300e0a32a48da28c4f7ec9d69bd1..8f1cef273f40075165310ad858c344b9fb71324f 100644 (file)
--- 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;