[Mono-dev] [PATCH] Managed Marshal.Copy implemantations

[Paolo Molaro]

On 12/20/07 Kornél Pál wrote:
>> It's an extra check because the shared methods will have Array as the
>> type and they could be misused.
>
> They are private so I prefer not to be used anywhere else than extra 
> checks. (I added comments before these methods about that.)

The check is cheap and it should go in.

BTW, please send patches as text attachments, not as binaries, thanks.

> Index: mono/mono/metadata/icall-def.h
> ===================================================================
> --- mono/mono/metadata/icall-def.h	(revision 91652)
> +++ mono/mono/metadata/icall-def.h	(working copy)
> @@ -91,15 +91,17 @@
>  ICALL(ARRAY_2, "Clone",            mono_array_clone)
>  ICALL(ARRAY_3, "CreateInstanceImpl",   ves_icall_System_Array_CreateInstanceImpl)
>  ICALL(ARRAY_4, "FastCopy",         ves_icall_System_Array_FastCopy)
> -ICALL(ARRAY_5, "GetGenericValueImpl", ves_icall_System_Array_GetGenericValueImpl)
> -ICALL(ARRAY_6, "GetLength",        ves_icall_System_Array_GetLength)
> -ICALL(ARRAY_7, "GetLowerBound",    ves_icall_System_Array_GetLowerBound)
> -ICALL(ARRAY_8, "GetRank",          ves_icall_System_Array_GetRank)
> -ICALL(ARRAY_9, "GetValue",         ves_icall_System_Array_GetValue)
> -ICALL(ARRAY_10, "GetValueImpl",     ves_icall_System_Array_GetValueImpl)
> -ICALL(ARRAY_11, "SetGenericValueImpl", ves_icall_System_Array_SetGenericValueImpl)
> -ICALL(ARRAY_12, "SetValue",         ves_icall_System_Array_SetValue)
> -ICALL(ARRAY_13, "SetValueImpl",     ves_icall_System_Array_SetValueImpl)
> +ICALL(ARRAY_5, "GetElementSize",   ves_icall_System_Array_GetElementSize)

There is no need to rewrite everything, just add your icall.

> @@ -395,7 +397,7 @@
>  ICALL(OBJ_1, "GetType", ves_icall_System_Object_GetType)
>  ICALL(OBJ_2, "InternalGetHashCode", mono_object_hash)
>  ICALL(OBJ_3, "MemberwiseClone", ves_icall_System_Object_MemberwiseClone)
> -ICALL(OBJ_4, "obj_address", ves_icall_System_Object_obj_address)
> +ICALL(OBJ_4, "UnsafeAddrOfPinnedObject", ves_icall_System_Object_UnsafeAddrOfPinnedObject)

Uneeded renaming, drop it.

> Index: mono/mono/metadata/icall.c
> ===================================================================
> --- mono/mono/metadata/icall.c	(revision 91652)
> +++ mono/mono/metadata/icall.c	(working copy)
> @@ -713,6 +713,22 @@
>  	memcpy (ea, value, esize);
>  }
>  
> +static gint
> +ves_icall_System_Array_GetElementSize (MonoObject *this)
> +{
> +	MONO_ARCH_SAVE_REGS;
> +
> +	return mono_array_element_size (this->vtable->klass);
> +}

This icall is not needed for this change: as I mentioned it's better to
add a size argument to copy_to_unmanaged etc.

> +static gint
> +ves_icall_System_Array_GetOffsetToArrayData (void)
> +{
> +	MONO_ARCH_SAVE_REGS;
> +
> +	return offsetof (MonoArray, vector);
> +}

This is not the icall that I suggested: you need to return an IntPtr
to the beginning of the elements:

static void*
array_get_array_data_ptr (MonoArray *array)
{
	return &array->vector [0];
}

> Index: mono/mono/mini/mini.c
> ===================================================================
> --- mono/mono/mini/mini.c	(revision 91652)
> +++ mono/mono/mini/mini.c	(working copy)
> @@ -3449,13 +3449,61 @@
>  			ins->inst_i0 = args [0];
>  			return ins;
>  #endif
> +		} else if (strcmp (cmethod->name, "UnsafeAddrOfPinnedObject") == 0) {
> +			return args [0];

This is not correct and this icall should not be optimized, since there
are no users.

>  		} else if (strcmp (cmethod->name, ".ctor") == 0) {
>   			MONO_INST_NEW (cfg, ins, OP_NOP);
>  			return ins;
>  		} else
>  			return NULL;
>  	} else if (cmethod->klass == mono_defaults.array_class) {
> - 		if (cmethod->name [0] != 'g')
> +		if (cmethod->name [0] == 'G') {
> +			if (strcmp (cmethod->name, "GetElementSize") == 0) {
> +				MonoInst *load_vtable, *load_klass;
> +
> +				MONO_INST_NEW (cfg, load_vtable, CEE_LDIND_I);
> +				if (G_STRUCT_OFFSET (MonoObject, vtable)) {

vtable will always be at offset 0.
Anyway, this icall should not be added for now.

> Index: mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs
> ===================================================================
> --- mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs	(revision 91652)
> +++ mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs	(working copy)
> @@ -1020,8 +1060,10 @@
>  				throw ex;
>  		}
>  
> -		[MethodImplAttribute(MethodImplOptions.InternalCall)]
> -		public extern static IntPtr UnsafeAddrOfPinnedArrayElement (Array arr, int index);
> +		public static unsafe IntPtr UnsafeAddrOfPinnedArrayElement (Array arr, int index)
> +		{
> +			return (IntPtr) ((byte*) object.UnsafeAddrOfPinnedObject (arr) + Array.GetOffsetToArrayData () + arr.GetElementSize () * index);
> +		}

I would leave this in unmanaged for now.
Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better