Skip to content

Commit 840e8fa

Browse files
BrzVladBrzVlad
authored
[mono][jit] Don't truncate the high bits when ldelema index is nint (#88986)
* [mono][interp] Remove dead code * [tests] Move issue to interp only * [mono][jit] Fix bounds check when index is nint Previous code was assuming index is i4. Remove MONO_ARCH_EMIT_BOUNDS_CHECK on amd64 since it was doing a I4 comparison, while the index reg is i8. Use MONO_EMIT_DEFAULT_BOUNDS_CHECK instead also on amd64. On llvm we might not be able to insert the sign extending because it apparently interferes with abcrem optimization. We therefore split OP_BOUNDS_CHECK into two separate opcodes, so, after abcrem, we know whether we have to insert the sext or not. * [mono][interp] Add missing sign extend Fix passing of i4 to wrapper accepting native int. The wrapper no longer does the sign extend in order to not lose high bits of native int. * feedback
1 parent 23e81d9 commit 840e8fa

File tree

9 files changed

+54
-54
lines changed

9 files changed

+54
-54
lines changed

‎src/mono/mono/mini/decompose.c

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1545,16 +1545,24 @@ mono_decompose_array_access_opts (MonoCompile *cfg)
15451545
ins->inst_imm, ins->flags);
15461546
MONO_ADD_INS (cfg->cbb, dest);
15471547
break;
1548-
case OP_BOUNDS_CHECK:
1548+
case OP_BOUNDS_CHECK: {
1549+
gboolean need_sext = ins->backend.need_sext;
15491550
MONO_EMIT_NULL_CHECK (cfg, ins->sreg1, FALSE);
15501551
if (COMPILE_LLVM (cfg)) {
1551-
int index2_reg = alloc_preg (cfg);
1552-
MONO_EMIT_NEW_UNALU (cfg, OP_SEXT_I4, index2_reg, ins->sreg2);
1552+
int index2_reg;
1553+
if (need_sext) {
1554+
index2_reg = alloc_preg (cfg);
1555+
MONO_EMIT_NEW_UNALU (cfg, OP_SEXT_I4, index2_reg, ins->sreg2);
1556+
} else {
1557+
index2_reg = ins->sreg2;
1558+
}
15531559
MONO_EMIT_DEFAULT_BOUNDS_CHECK (cfg, ins->sreg1, GINT32_TO_UINT32(ins->inst_imm), index2_reg, ins->flags & MONO_INST_FAULT, ins->inst_p0);
15541560
} else {
1561+
g_assert (!need_sext);
15551562
MONO_ARCH_EMIT_BOUNDS_CHECK (cfg, ins->sreg1, ins->inst_imm, ins->sreg2, ins->inst_p0);
15561563
}
15571564
break;
1565+
}
15581566
case OP_NEWARR: {
15591567
ERROR_DECL (vt_error);
15601568
MonoClass *array_class = mono_class_create_array (ins->inst_newa_class, 1);

‎src/mono/mono/mini/interp/interp.c

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1211,27 +1211,6 @@ ves_array_calculate_index (MonoArray *ao, stackval *sp, gboolean safe)
12111211
return pos;
12121212
}
12131213

1214-
static MonoException*
1215-
ves_array_get (InterpFrame *frame, stackval *sp, stackval *retval, MonoMethodSignature *sig, gboolean safe)
1216-
{
1217-
MonoObject *o = sp->data.o;
1218-
MonoArray *ao = (MonoArray *) o;
1219-
MonoClass *ac = o->vtable->klass;
1220-
1221-
g_assert (m_class_get_rank (ac) >= 1);
1222-
1223-
gint32 pos = ves_array_calculate_index (ao, sp + 1, safe);
1224-
if (pos == -1)
1225-
return mono_get_exception_index_out_of_range ();
1226-
1227-
gint32 esize = mono_array_element_size (ac);
1228-
gconstpointer ea = mono_array_addr_with_size_fast (ao, esize, pos);
1229-
1230-
MonoType *mt = sig->ret;
1231-
stackval_from_data (mt, retval, ea, FALSE);
1232-
return NULL;
1233-
}
1234-
12351214
static MonoException*
12361215
ves_array_element_address (InterpFrame *frame, MonoClass *required_type, MonoArray *ao, gpointer *ret, stackval *sp, gboolean needs_typecheck)
12371216
{

‎src/mono/mono/mini/intrinsics.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -356,9 +356,10 @@ emit_span_intrinsics (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSignature
356356
/* Similar to mini_emit_ldelema_1_ins () */
357357
int size = mono_class_array_element_size (param_class);
358358

359-
int index_reg = mini_emit_sext_index_reg (cfg, args [1]);
359+
gboolean need_sext;
360+
int index_reg = mini_emit_sext_index_reg (cfg, args [1], &need_sext);
360361

361-
mini_emit_bounds_check_offset (cfg, span_reg, length_field->offset - MONO_ABI_SIZEOF (MonoObject), index_reg, NULL);
362+
mini_emit_bounds_check_offset (cfg, span_reg, length_field->offset - MONO_ABI_SIZEOF (MonoObject), index_reg, NULL, need_sext);
362363

363364
// FIXME: Sign extend index ?
364365

@@ -849,7 +850,7 @@ mini_emit_inst_for_method (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSign
849850
#else
850851
index_reg = args [1]->dreg;
851852
#endif
852-
MONO_EMIT_BOUNDS_CHECK (cfg, args [0]->dreg, MonoString, length, index_reg);
853+
MONO_EMIT_BOUNDS_CHECK (cfg, args [0]->dreg, MonoString, length, index_reg, FALSE);
853854

854855
#if defined(TARGET_X86) || defined(TARGET_AMD64)
855856
EMIT_NEW_X86_LEA (cfg, ins, args [0]->dreg, index_reg, 1, MONO_STRUCT_OFFSET (MonoString, chars));

‎src/mono/mono/mini/ir-emit.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -971,11 +971,12 @@ static int ccount = 0;
971971
#endif
972972

973973
static inline void
974-
mini_emit_bounds_check_offset (MonoCompile *cfg, int array_reg, int array_length_offset, int index_reg, const char *ex_name)
974+
mini_emit_bounds_check_offset (MonoCompile *cfg, int array_reg, int array_length_offset, int index_reg, const char *ex_name, gboolean need_sext)
975975
{
976976
if (!(cfg->opt & MONO_OPT_UNSAFE)) {
977977
ex_name = ex_name ? ex_name : "IndexOutOfRangeException";
978978
if (!(cfg->opt & MONO_OPT_ABCREM)) {
979+
g_assert (!need_sext);
979980
MONO_EMIT_NULL_CHECK (cfg, array_reg, FALSE);
980981
if (COMPILE_LLVM (cfg))
981982
MONO_EMIT_DEFAULT_BOUNDS_CHECK ((cfg), (array_reg), GINT_TO_UINT(array_length_offset), (index_reg), TRUE, ex_name);
@@ -988,6 +989,7 @@ mini_emit_bounds_check_offset (MonoCompile *cfg, int array_reg, int array_length
988989
ins->sreg2 = index_reg;
989990
ins->inst_p0 = (gpointer)ex_name;
990991
ins->inst_imm = (array_length_offset);
992+
ins->backend.need_sext = need_sext;
991993
ins->flags |= MONO_INST_FAULT;
992994
MONO_ADD_INS ((cfg)->cbb, ins);
993995
(cfg)->flags |= MONO_CFG_NEEDS_DECOMPOSE;
@@ -1002,8 +1004,8 @@ mini_emit_bounds_check_offset (MonoCompile *cfg, int array_reg, int array_length
10021004
* array_length_field is the field in the previous struct with the length
10031005
* index_reg is the vreg holding the index
10041006
*/
1005-
#define MONO_EMIT_BOUNDS_CHECK(cfg, array_reg, array_type, array_length_field, index_reg) do { \
1006-
mini_emit_bounds_check_offset ((cfg), (array_reg), MONO_STRUCT_OFFSET (array_type, array_length_field), (index_reg), NULL); \
1007+
#define MONO_EMIT_BOUNDS_CHECK(cfg, array_reg, array_type, array_length_field, index_reg, need_sext) do { \
1008+
mini_emit_bounds_check_offset ((cfg), (array_reg), MONO_STRUCT_OFFSET (array_type, array_length_field), (index_reg), NULL, need_sext); \
10071009
} while (0)
10081010

10091011
#endif

‎src/mono/mono/mini/method-to-ir.c

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4211,20 +4211,27 @@ mini_field_access_needs_cctor_run (MonoCompile *cfg, MonoMethod *method, MonoCla
42114211
}
42124212

42134213
int
4214-
mini_emit_sext_index_reg (MonoCompile *cfg, MonoInst *index)
4214+
mini_emit_sext_index_reg (MonoCompile *cfg, MonoInst *index, gboolean *need_sext)
42154215
{
42164216
int index_reg = index->dreg;
42174217
int index2_reg;
42184218

4219+
*need_sext = FALSE;
4220+
42194221
#if SIZEOF_REGISTER == 8
4222+
// If index is not I4 don't sign extend otherwise we lose high word
4223+
if (index->type != STACK_I4)
4224+
return index_reg;
4225+
42204226
/* The array reg is 64 bits but the index reg is only 32 */
4221-
if (COMPILE_LLVM (cfg)) {
4227+
if (cfg->opt & MONO_OPT_ABCREM) {
42224228
/*
42234229
* abcrem can't handle the OP_SEXT_I4, so add this after abcrem,
42244230
* during OP_BOUNDS_CHECK decomposition, and in the implementation
42254231
* of OP_X86_LEA for llvm.
42264232
*/
42274233
index2_reg = index_reg;
4234+
*need_sext = TRUE;
42284235
} else {
42294236
index2_reg = alloc_preg (cfg);
42304237
MONO_EMIT_NEW_UNALU (cfg, OP_SEXT_I4, index2_reg, index_reg);
@@ -4259,7 +4266,9 @@ mini_emit_ldelema_1_ins (MonoCompile *cfg, MonoClass *klass, MonoInst *arr, Mono
42594266
mult_reg = alloc_preg (cfg);
42604267
array_reg = arr->dreg;
42614268

4262-
realidx2_reg = index2_reg = mini_emit_sext_index_reg (cfg, index);
4269+
gboolean need_sext;
4270+
4271+
realidx2_reg = index2_reg = mini_emit_sext_index_reg (cfg, index, &need_sext);
42634272

42644273
if (bounded) {
42654274
bounds_reg = alloc_preg (cfg);
@@ -4283,7 +4292,7 @@ mini_emit_ldelema_1_ins (MonoCompile *cfg, MonoClass *klass, MonoInst *arr, Mono
42834292
}
42844293

42854294
if (bcheck)
4286-
MONO_EMIT_BOUNDS_CHECK (cfg, array_reg, MonoArray, max_length, realidx2_reg);
4295+
MONO_EMIT_BOUNDS_CHECK (cfg, array_reg, MonoArray, max_length, realidx2_reg, need_sext);
42874296

42884297
#if defined(TARGET_X86) || defined(TARGET_AMD64)
42894298
if (size == 1 || size == 2 || size == 4 || size == 8) {
@@ -4444,8 +4453,18 @@ mini_emit_array_store (MonoCompile *cfg, MonoClass *klass, MonoInst **sp, gboole
44444453
if (sp [2]->type != STACK_OBJ)
44454454
return NULL;
44464455

4456+
MonoInst *index_ins = sp [1];
4457+
#if SIZEOF_REGISTER == 8
4458+
if (sp [1]->type == STACK_I4) {
4459+
// stelemref wrapper receives index as native int, sign extend it
4460+
guint32 dreg = alloc_preg (cfg);
4461+
guint32 sreg = index_ins->dreg;
4462+
EMIT_NEW_UNALU (cfg, index_ins, OP_SEXT_I4, dreg, sreg);
4463+
}
4464+
#endif
4465+
44474466
iargs [2] = sp [2];
4448-
iargs [1] = sp [1];
4467+
iargs [1] = index_ins;
44494468
iargs [0] = sp [0];
44504469

44514470
MonoClass *array_class = sp [0]->klass;
@@ -4483,7 +4502,7 @@ mini_emit_array_store (MonoCompile *cfg, MonoClass *klass, MonoInst **sp, gboole
44834502
MONO_EMIT_NEW_UNALU (cfg, OP_ZEXT_I4, index_reg, index_reg);
44844503

44854504
if (safety_checks)
4486-
MONO_EMIT_BOUNDS_CHECK (cfg, array_reg, MonoArray, max_length, index_reg);
4505+
MONO_EMIT_BOUNDS_CHECK (cfg, array_reg, MonoArray, max_length, index_reg, FALSE);
44874506
EMIT_NEW_STORE_MEMBASE_TYPE (cfg, ins, m_class_get_byval_arg (klass), array_reg, (target_mgreg_t)offset, sp [2]->dreg);
44884507
} else {
44894508
MonoInst *addr = mini_emit_ldelema_1_ins (cfg, klass, sp [0], sp [1], safety_checks, FALSE);
@@ -10598,7 +10617,7 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
1059810617
if (SIZEOF_REGISTER == 8 && COMPILE_LLVM (cfg))
1059910618
MONO_EMIT_NEW_UNALU (cfg, OP_ZEXT_I4, index_reg, index_reg);
1060010619

10601-
MONO_EMIT_BOUNDS_CHECK (cfg, array_reg, MonoArray, max_length, index_reg);
10620+
MONO_EMIT_BOUNDS_CHECK (cfg, array_reg, MonoArray, max_length, index_reg, FALSE);
1060210621
EMIT_NEW_LOAD_MEMBASE_TYPE (cfg, ins, m_class_get_byval_arg (klass), array_reg, (target_mgreg_t)offset);
1060310622
} else {
1060410623
addr = mini_emit_ldelema_1_ins (cfg, klass, sp [0], sp [1], TRUE, FALSE);

‎src/mono/mono/mini/mini-amd64.h

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -480,16 +480,6 @@ typedef struct {
480480
/* Used for optimization, not complete */
481481
#define MONO_ARCH_IS_OP_MEMBASE(opcode) ((opcode) == OP_X86_PUSH_MEMBASE)
482482

483-
#define MONO_ARCH_EMIT_BOUNDS_CHECK(cfg, array_reg, offset, index_reg, ex_name) do { \
484-
MonoInst *inst; \
485-
MONO_INST_NEW ((cfg), inst, OP_AMD64_ICOMPARE_MEMBASE_REG); \
486-
inst->inst_basereg = array_reg; \
487-
inst->inst_offset = offset; \
488-
inst->sreg2 = index_reg; \
489-
MONO_ADD_INS ((cfg)->cbb, inst); \
490-
MONO_EMIT_NEW_COND_EXC (cfg, LE_UN, ex_name); \
491-
} while (0)
492-
493483
// Does the ABI have a volatile non-parameter register, so tailcall
494484
// can pass context to generics or interfaces?
495485
#define MONO_ARCH_HAVE_VOLATILE_NON_PARAM_REGISTER 1

‎src/mono/mono/mini/mini.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -753,6 +753,7 @@ struct MonoInst {
753753
gpointer data;
754754
gint shift_amount;
755755
gboolean is_pinvoke; /* for variables in the unmanaged marshal format */
756+
gboolean need_sext; /* for OP_BOUNDS_CHECK */
756757
gboolean record_cast_details; /* For CEE_CASTCLASS */
757758
MonoInst *spill_var; /* for OP_MOVE_I4_TO_F/F_TO_I4 and OP_FCONV_TO_R8_X */
758759
guint16 source_opcode; /*OP_XCONV_R8_TO_I4 needs to know which op was used to do proper widening*/
@@ -2316,7 +2317,7 @@ void mini_emit_memset (MonoCompile *cfg, int destreg, int offset, i
23162317
void mini_emit_stobj (MonoCompile *cfg, MonoInst *dest, MonoInst *src, MonoClass *klass, gboolean native);
23172318
void mini_emit_initobj (MonoCompile *cfg, MonoInst *dest, const guchar *ip, MonoClass *klass);
23182319
void mini_emit_init_rvar (MonoCompile *cfg, int dreg, MonoType *rtype);
2319-
int mini_emit_sext_index_reg (MonoCompile *cfg, MonoInst *index);
2320+
int mini_emit_sext_index_reg (MonoCompile *cfg, MonoInst *index, gboolean *need_sext);
23202321
MonoInst* mini_emit_ldelema_1_ins (MonoCompile *cfg, MonoClass *klass, MonoInst *arr, MonoInst *index, gboolean bcheck, gboolean bounded);
23212322
MonoInst* mini_emit_get_gsharedvt_info_klass (MonoCompile *cfg, MonoClass *klass, MonoRgctxInfoType rgctx_type);
23222323
MonoInst* mini_emit_get_rgctx_method (MonoCompile *cfg, int context_used,

‎src/mono/mono/mini/simd-intrinsics.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3240,12 +3240,12 @@ emit_sys_numerics_vector_t (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSig
32403240
}
32413241

32423242
/* Emit bounds check for the index (index >= 0) */
3243-
mini_emit_bounds_check_offset (cfg, array_ins->dreg, MONO_STRUCT_OFFSET (MonoArray, max_length), index_ins->dreg, "ArgumentOutOfRangeException");
3243+
mini_emit_bounds_check_offset (cfg, array_ins->dreg, MONO_STRUCT_OFFSET (MonoArray, max_length), index_ins->dreg, "ArgumentOutOfRangeException", FALSE);
32443244

32453245
/* Emit bounds check for the end (index + len - 1 < array length) */
32463246
end_index_reg = alloc_ireg (cfg);
32473247
EMIT_NEW_BIALU_IMM (cfg, ins, OP_IADD_IMM, end_index_reg, index_ins->dreg, len - 1);
3248-
mini_emit_bounds_check_offset (cfg, array_ins->dreg, MONO_STRUCT_OFFSET (MonoArray, max_length), end_index_reg, "ArgumentOutOfRangeException");
3248+
mini_emit_bounds_check_offset (cfg, array_ins->dreg, MONO_STRUCT_OFFSET (MonoArray, max_length), end_index_reg, "ArgumentOutOfRangeException", FALSE);
32493249

32503250
/* Load the array slice into the simd reg */
32513251
ldelema_ins = mini_emit_ldelema_1_ins (cfg, mono_class_from_mono_type_internal (etype), array_ins, index_ins, FALSE, FALSE);
@@ -3274,7 +3274,7 @@ emit_sys_numerics_vector_t (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSig
32743274
}
32753275

32763276
/* CopyTo () does complicated argument checks */
3277-
mini_emit_bounds_check_offset (cfg, array_ins->dreg, MONO_STRUCT_OFFSET (MonoArray, max_length), index_ins->dreg, "ArgumentOutOfRangeException");
3277+
mini_emit_bounds_check_offset (cfg, array_ins->dreg, MONO_STRUCT_OFFSET (MonoArray, max_length), index_ins->dreg, "ArgumentOutOfRangeException", FALSE);
32783278
end_index_reg = alloc_ireg (cfg);
32793279
int len_reg = alloc_ireg (cfg);
32803280
MONO_EMIT_NEW_LOAD_MEMBASE_OP_FLAGS (cfg, OP_LOADI4_MEMBASE, len_reg, array_ins->dreg, MONO_STRUCT_OFFSET (MonoArray, max_length), MONO_INST_INVARIANT_LOAD);

‎src/tests/issues.targets

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1204,9 +1204,6 @@
12041204
<ExcludeList Include="$(XunitTestBinBase)/Interop/PInvoke/Int128/Int128TestFieldLayout/*">
12051205
<Issue>https://github.com/dotnet/runtime/issues/74223</Issue>
12061206
</ExcludeList>
1207-
<ExcludeList Include = "$(XUnitTestBinBase)/JIT/Directed/Arrays/nintindexoutofrange/**">
1208-
<Issue>https://github.com/dotnet/runtime/issues/71656</Issue>
1209-
</ExcludeList>
12101207
<ExcludeList Include = "$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/Sse2.X64/StoreNonTemporal_r/**">
12111208
<Issue>https://github.com/dotnet/runtime/issues/54176</Issue>
12121209
</ExcludeList>
@@ -2234,6 +2231,9 @@
22342231
<ExcludeList Include = "$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_74635/Runtime_74635_1/**">
22352232
<Issue>https://github.com/dotnet/runtime/issues/74687</Issue>
22362233
</ExcludeList>
2234+
<ExcludeList Include = "$(XUnitTestBinBase)/JIT/Directed/Arrays/nintindexoutofrange/**">
2235+
<Issue>https://github.com/dotnet/runtime/issues/71656</Issue>
2236+
</ExcludeList>
22372237
<!-- End interpreter issues -->
22382238
</ItemGroup>
22392239

0 commit comments

Comments
 (0)