Merge pull request #3085 from lateralusX/jlorenss/win-x64-pinvoke-empty-struct

Fix for non GCC win x64 calling convention when passing empty structs over pinvoke.

diff --git a/mono/mini/mini-amd64.c b/mono/mini/mini-amd64.c
index 3e59c9da16d42fd0f6aae2165428f3ff2ba3d523..4771ab861844e3f850c96be5d0d2470403076d39 100644 (file)
--- a/mono/mini/mini-amd64.c
+++ b/mono/mini/mini-amd64.c
@@ -392,6 +392,17 @@ add_valuetype_win64 (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);
+
+       /*
+       * 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 (0 == size && MONO_TYPE_ISSTRUCT (type) && sig->pinvoke)
+               ainfo->pass_empty_struct = TRUE;
+       
        if (!sig->pinvoke)
                pass_on_stack = TRUE;
 
@@ -448,13 +459,17 @@ add_valuetype_win64 (MonoMethodSignature *sig, ArgInfo *ainfo, MonoType *type,
        } else {
                g_assert (info);
 
-               if (!fields) {
+               /*Only drop value type if its not an empty struct as input that must be represented in call*/
+               if ((!fields && !ainfo->pass_empty_struct) || (!fields && ainfo->pass_empty_struct && is_return)) {
                        ainfo->storage = ArgValuetypeInReg;
                        ainfo->pair_storage [0] = ainfo->pair_storage [1] = ArgNone;
                        return;
                }
 
                switch (info->native_size) {
+               case 0:
+                       g_assert (!fields && MONO_TYPE_ISSTRUCT (type) && !is_return);
+                       break;
                case 1: case 2: case 4: case 8:
                        break;
                default:
@@ -487,7 +502,11 @@ add_valuetype_win64 (MonoMethodSignature *sig, ArgInfo *ainfo, MonoType *type,
                guint32 align;
                ArgumentClass class1;
 
-               if (nfields == 0)
+               if (nfields == 0 && ainfo->pass_empty_struct) {
+                       g_assert (!fields && !is_return);
+                       class1 = ARG_CLASS_INTEGER;
+               }
+               else if (nfields == 0)
                        class1 = ARG_CLASS_MEMORY;
                else
                        class1 = ARG_CLASS_NO_CLASS;
@@ -586,6 +605,7 @@ 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 (!sig->pinvoke && ((is_return && (size == 8)) || (!is_return && (size <= 16)))) {
                /* We pass and return vtypes of size 8 in a register */
        } else if (!sig->pinvoke || (size == 0) || (size > 16)) {
@@ -2112,7 +2132,7 @@ mono_arch_emit_call (MonoCompile *cfg, MonoCallInst *call)
                                /* Continue normally */
                        }
 
-                       if (size > 0) {
+                       if (size > 0 || ainfo->pass_empty_struct) {
                                MONO_INST_NEW (cfg, arg, OP_OUTARG_VT);
                                arg->sreg1 = in->dreg;
                                arg->klass = mono_class_from_mono_type (t);
@@ -2210,21 +2230,28 @@ mono_arch_emit_outarg_vt (MonoCompile *cfg, MonoInst *ins, MonoInst *src)
                        if (ainfo->pair_storage [part] == ArgNone)
                                continue;
 
-                       MONO_INST_NEW (cfg, load, arg_storage_to_load_membase (ainfo->pair_storage [part]));
-                       load->inst_basereg = src->dreg;
-                       load->inst_offset = part * sizeof(mgreg_t);
+                       if (ainfo->pass_empty_struct) {
+                               //Pass empty struct value as 0 on platforms representing empty structs as 1 byte.
+                               NEW_ICONST (cfg, load, 0);
+                       }
+                       else {
+                               MONO_INST_NEW (cfg, load, arg_storage_to_load_membase (ainfo->pair_storage [part]));
+                               load->inst_basereg = src->dreg;
+                               load->inst_offset = part * sizeof(mgreg_t);
 
-                       switch (ainfo->pair_storage [part]) {
-                       case ArgInIReg:
-                               load->dreg = mono_alloc_ireg (cfg);
-                               break;
-                       case ArgInDoubleSSEReg:
-                       case ArgInFloatSSEReg:
-                               load->dreg = mono_alloc_freg (cfg);
-                               break;
-                       default:
-                               g_assert_not_reached ();
+                               switch (ainfo->pair_storage [part]) {
+                               case ArgInIReg:
+                                       load->dreg = mono_alloc_ireg (cfg);
+                                       break;
+                               case ArgInDoubleSSEReg:
+                               case ArgInFloatSSEReg:
+                                       load->dreg = mono_alloc_freg (cfg);
+                                       break;
+                               default:
+                                       g_assert_not_reached ();
+                               }
                        }
+
                        MONO_ADD_INS (cfg->cbb, load);
 
                        add_outarg_reg (cfg, call, ainfo->pair_storage [part], ainfo->pair_regs [part], load);

diff --git a/mono/mini/mini-amd64.h b/mono/mini/mini-amd64.h
index 68aaf3925464c1d3b7d4b27adef32eafa586d2df..11fc30a435f28b08366bd6cae009fe21602147d4 100644 (file)
--- a/mono/mini/mini-amd64.h
+++ b/mono/mini/mini-amd64.h
@@ -269,6 +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.
 } ArgInfo;
 
 typedef struct {

diff --git a/mono/tests/libtest.c b/mono/tests/libtest.c
index 650e7716e583d8af4318a59a900c7ad8b742780c..1f2930da9655300e0a32a48da28c4f7ec9d69bd1 100644 (file)
--- a/mono/tests/libtest.c
+++ b/mono/tests/libtest.c
@@ -1228,14 +1228,29 @@ mono_test_marshal_stringbuilder_ref (char **s)
        return 0;
 }
 
+#ifdef __GNUC__
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wc++-compat"
+#endif
+
+/*
+* 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. On Windows x64 structs will always be represented in the call
+* meaning that an empty struct must have a representation in the callee in order to correctly follow the ABI used by the
+* C/C++ standard and the runtime.
+*/
 typedef struct {
-#ifndef __GNUC__
+#if !defined(__GNUC__) || (defined(TARGET_WIN32) && defined(TARGET_AMD64))
     char a;
 #endif
 } EmptyStruct;
+
+#ifdef __GNUC__
 #pragma GCC diagnostic pop
+#endif
 
 LIBTEST_API int STDCALL 
 mono_test_marshal_empty_string_array (char **array)