Skip to content

Commit

Permalink
Fix missing array ctor(int32,int32) test failures. (dotnet#34764)
Browse files Browse the repository at this point in the history 
According to ECMA, VES should add the following methods to arrays:

* A constructor that takes a sequence of int32 arguments, one for each dimension of the array, that specify the number of elements in each dimension beginning with the first dimension. A lower bound of zero is assumed.

* A constructor that takes twice as many int32 arguments as there are dimensions of the array. These arguments occur in pairs—one pair per dimension—with the first argument of each pair specifying the lower bound for that dimension, and the second argument specifying the total number of elements in that dimension.

* Get.

* Set.

* Address.

In Mono's case we only added the `ctor (int32, int32)` for rank 1 arrays if they were "jagged" array. Array with rank 1-4 executed an optimized code path always using the one int32 per rank constructor.

This doesn't work in cases where the array is constructed using both a length and a lower bound pattern, like:

`newobj instance void int32[10000...10005]::.ctor(int32, int32)`

this would trigger a method not found exception, since its not a jagged array, Mono wouldn't add the `ctor(int32, int32)` constructor for this type.

Fix will make sure we follow ECMA's definition of this and fix code to use right instance methods of arrays. Code also adds an assert to trap if a jagged array is created through this code path
(shouldn't happen) since current implementation would have handled it as
a 2 ranked array.

Fix also adds several tests into iltests.il allocating different variations of arrays using lower/upper bounds and validating length, rank, upper/lower bounds of allocated arrays.

Co-authored-by: lateralusX <[email protected]>
  • Loading branch information
@monojenkins @lateralusX
monojenkins and lateralusX authored Apr 15, 2020
1 parent dd22ce8 commit fdf812f
Show file tree
Hide file tree
Showing 3 changed files with 185 additions and 36 deletions.
68 changes: 46 additions & 22 deletions src/mono/mono/metadata/class-init.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
#undef REALLY_INCLUDE_CLASS_DEF
#endif


gboolean mono_print_vtable = FALSE;
gboolean mono_align_small_structs = FALSE;
#ifdef ENABLE_NETCORE
Expand Down Expand Up @@ -4237,6 +4236,48 @@ generic_array_methods (MonoClass *klass)
return generic_array_method_num;
}

static int array_get_method_count (MonoClass *klass)
{
MonoType *klass_byval_arg = m_class_get_byval_arg (klass);
if (klass_byval_arg->type == MONO_TYPE_ARRAY)
/* Regular array */
/* ctor([int32]*rank) */
/* ctor([int32]*rank*2) */
/* Get */
/* Set */
/* Address */
return 5;
else if (klass_byval_arg->type == MONO_TYPE_SZARRAY && klass->rank == 1 && klass->element_class->rank)
/* Jagged arrays are typed as MONO_TYPE_SZARRAY but have an extra ctor in .net which creates an array of arrays */
/* ctor([int32]) */
/* ctor([int32], [int32]) */
/* Get */
/* Set */
/* Address */
return 5;
else
/* Vectors don't have additional constructor since a zero lower bound is assumed */
/* ctor([int32]*rank) */
/* Get */
/* Set */
/* Address */
return 4;
}

static gboolean array_supports_additional_ctor_method (MonoClass *klass)
{
MonoType *klass_byval_arg = m_class_get_byval_arg (klass);
if (klass_byval_arg->type == MONO_TYPE_ARRAY)
/* Regular array */
return TRUE;
else if (klass_byval_arg->type == MONO_TYPE_SZARRAY && klass->rank == 1 && klass->element_class->rank)
/* Jagged array */
return TRUE;
else
/* Vector */
return FALSE;
}

/*
* Global pool of interface IDs, represented as a bitset.
* LOCKING: Protected by the classes lock.
Expand Down Expand Up @@ -4522,7 +4563,7 @@ mono_class_init_internal (MonoClass *klass)
}

if (klass->rank) {
array_method_count = 3 + (klass->rank > 1? 2: 1);
array_method_count = array_get_method_count (klass);

if (klass->interface_count) {
int count_generic = generic_array_methods (klass);
Expand Down Expand Up @@ -4951,18 +4992,12 @@ mono_class_setup_methods (MonoClass *klass)
MonoMethodSignature *sig;
int count_generic = 0, first_generic = 0;
int method_num = 0;
gboolean jagged_ctor = FALSE;

count = 3 + (klass->rank > 1? 2: 1);
count = array_get_method_count (klass);

mono_class_setup_interfaces (klass, error);
g_assert (is_ok (error)); /*FIXME can this fail for array types?*/

if (klass->rank == 1 && klass->element_class->rank) {
jagged_ctor = TRUE;
count ++;
}

if (klass->interface_count) {
count_generic = generic_array_methods (klass);
first_generic = count;
Expand All @@ -4980,7 +5015,8 @@ mono_class_setup_methods (MonoClass *klass)

amethod = create_array_method (klass, ".ctor", sig);
methods [method_num++] = amethod;
if (klass->rank > 1) {

if (array_supports_additional_ctor_method (klass)) {
sig = mono_metadata_signature_alloc (klass->image, klass->rank * 2);
sig->ret = mono_get_void_type ();
sig->pinvoke = TRUE;
Expand All @@ -4992,18 +5028,6 @@ mono_class_setup_methods (MonoClass *klass)
methods [method_num++] = amethod;
}

if (jagged_ctor) {
/* Jagged arrays have an extra ctor in .net which creates an array of arrays */
sig = mono_metadata_signature_alloc (klass->image, klass->rank + 1);
sig->ret = mono_get_void_type ();
sig->pinvoke = TRUE;
sig->hasthis = TRUE;
for (i = 0; i < klass->rank + 1; ++i)
sig->params [i] = mono_get_int32_type ();
amethod = create_array_method (klass, ".ctor", sig);
methods [method_num++] = amethod;
}

/* element Get (idx11, [idx2, ...]) */
sig = mono_metadata_signature_alloc (klass->image, klass->rank);
sig->ret = m_class_get_byval_arg (m_class_get_element_class (klass));
Expand Down
118 changes: 117 additions & 1 deletion src/mono/mono/mini/iltests.il
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

.method static public int32 Main(string[] args) il managed {
.entrypoint

ldtoken Tests
call class [mscorlib]System.Type [mscorlib]System.Type::GetTypeFromHandle(valuetype [mscorlib]System.RuntimeTypeHandle)
ldarg.0
Expand Down Expand Up @@ -3557,4 +3557,120 @@ L3:
IL_0024: ret
}

.method private hidebysig static int32 validate_alloc_array_length_lbound(class [mscorlib]System.Array test_array) cil managed
{
// Code size 88 (0x58)
.maxstack 2
.locals init ([0] bool V_0, [1] int32 V_1, [2] bool V_2, [3] bool V_3, [4] bool V_4)
IL_0000: nop
IL_0001: ldarg.0
IL_0002: callvirt instance int32 [mscorlib]System.Array::get_Length()
IL_0007: ldc.i4.6
IL_0008: ceq
IL_000a: ldc.i4.0
IL_000b: ceq
IL_000d: stloc.0
IL_000e: ldloc.0
IL_000f: brfalse.s IL_0015
IL_0011: ldc.i4.1
IL_0012: stloc.1
IL_0013: br.s IL_0056
IL_0015: ldarg.0
IL_0016: callvirt instance int32 [mscorlib]System.Array::get_Rank()
IL_001b: ldc.i4.1
IL_001c: ceq
IL_001e: ldc.i4.0
IL_001f: ceq
IL_0021: stloc.2
IL_0022: ldloc.2
IL_0023: brfalse.s IL_0029
IL_0025: ldc.i4.2
IL_0026: stloc.1
IL_0027: br.s IL_0056
IL_0029: ldarg.0
IL_002a: ldc.i4.0
IL_002b: callvirt instance int32 [mscorlib]System.Array::GetLowerBound(int32)
IL_0030: ldc.i4 0x2710
IL_0031: cgt.un
IL_0033: stloc.3
IL_0034: ldloc.3
IL_0035: brfalse.s IL_003b
IL_0037: ldc.i4.3
IL_0038: stloc.1
IL_0039: br.s IL_0056
IL_003b: ldarg.0
IL_003c: ldc.i4.0
IL_003d: callvirt instance int32 [mscorlib]System.Array::GetUpperBound(int32)
IL_0042: ldc.i4 0x2715
IL_0043: ceq
IL_0045: ldc.i4.0
IL_0046: ceq
IL_0048: stloc.s V_4
IL_004a: ldloc.s V_4
IL_004c: brfalse.s IL_0052
IL_004e: ldc.i4.4
IL_004f: stloc.1
IL_0050: br.s IL_0056
IL_0052: ldc.i4.0
IL_0053: stloc.1
IL_0054: br.s IL_0056
IL_0056: ldloc.1
IL_0057: ret
}

.method public hidebysig static int32 test_0_alloc_array_int16_length_lbound() cil managed
{
.maxstack 2
.locals init ([0] int16[] test_array, [1] int32 V_1)
ldc.i4 10000
ldc.i4 6
newobj instance void int16[10000...10005]::.ctor(int32, int32)
call int32 class Tests::validate_alloc_array_length_lbound(class [mscorlib]System.Array)
ret
}

.method public hidebysig static int32 test_0_alloc_array_int32_length_lbound() cil managed
{
.maxstack 2
.locals init ([0] int32[] test_array, [1] int32 V_1)
ldc.i4 10000
ldc.i4 6
newobj instance void int32[10000...10005]::.ctor(int32, int32)
call int32 class Tests::validate_alloc_array_length_lbound(class [mscorlib]System.Array)
ret
}

.method public hidebysig static int32 test_0_alloc_array_int64_length_lbound() cil managed
{
.maxstack 2
.locals init ([0] int64[] test_array, [1] int32 V_1)
ldc.i4 10000
ldc.i4 6
newobj instance void int64[10000...10005]::.ctor(int32, int32)
call int32 class Tests::validate_alloc_array_length_lbound(class [mscorlib]System.Array)
ret
}

.method public hidebysig static int32 test_0_alloc_array_float32_length_lbound() cil managed
{
.maxstack 2
.locals init ([0] float32[] test_array, [1] int32 V_1)
ldc.i4 10000
ldc.i4 6
newobj instance void float32[10000...10005]::.ctor(int32, int32)
call int32 class Tests::validate_alloc_array_length_lbound(class [mscorlib]System.Array)
ret
}

.method public hidebysig static int32 test_0_alloc_array_float64_length_lbound() cil managed
{
.maxstack 2
.locals init ([0] float64[] test_array, [1] int32 V_1)
ldc.i4 10000
ldc.i4 6
newobj instance void float64[10000...10005]::.ctor(int32, int32)
call int32 class Tests::validate_alloc_array_length_lbound(class [mscorlib]System.Array)
ret
}

}
35 changes: 22 additions & 13 deletions src/mono/mono/mini/method-to-ir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8722,19 +8722,29 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
if (mini_class_is_system_array (cmethod->klass)) {
*sp = emit_get_rgctx_method (cfg, context_used,
cmethod, MONO_RGCTX_INFO_METHOD);
/* Optimize the common cases */
MonoJitICallId function = MONO_JIT_ICALL_ZeroIsReserved;;
MonoJitICallId function = MONO_JIT_ICALL_ZeroIsReserved;
int n = fsig->param_count;
switch (n) {
case 1: function = MONO_JIT_ICALL_mono_array_new_1;
break;
case 2: function = MONO_JIT_ICALL_mono_array_new_2;
break;
case 3: function = MONO_JIT_ICALL_mono_array_new_3;
break;
case 4: function = MONO_JIT_ICALL_mono_array_new_4;
break;
default:
/* Optimize the common cases, use ctor using length for each rank (no lbound). */
if (n == m_class_get_rank (cmethod->klass)) {
switch (n) {
case 1: function = MONO_JIT_ICALL_mono_array_new_1;
break;
case 2: function = MONO_JIT_ICALL_mono_array_new_2;
break;
case 3: function = MONO_JIT_ICALL_mono_array_new_3;
break;
case 4: function = MONO_JIT_ICALL_mono_array_new_4;
break;
default:
break;
}
}

/* Instancing jagged arrays should not end up here since ctor (int32, int32) for an array with rank 1 represent lenght and lbound. */
g_assert (!(m_class_get_rank (cmethod->klass) == 1 && fsig->param_count == 2 && m_class_get_rank (m_class_get_element_class (cmethod->klass))));

/* Regular case, rank > 4 or legnth, lbound specified per rank. */
if (function == MONO_JIT_ICALL_ZeroIsReserved) {
// FIXME Maximum value of param_count? Realistically 64. Fits in imm?
if (!array_new_localalloc_ins) {
MONO_INST_NEW (cfg, array_new_localalloc_ins, OP_LOCALLOC_IMM);
Expand All @@ -8755,7 +8765,6 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
sp [2] = ins;
// FIXME Adjust sp by n - 3? Attempts failed.
function = MONO_JIT_ICALL_mono_array_new_n_icall;
break;
}
alloc = mono_emit_jit_icall_id (cfg, function, sp);
} else if (cmethod->string_ctor) {
Expand Down

0 comments on commit fdf812f

Please sign in to comment.