Skip to content

Commit

Permalink
[mono][jit] Don't truncate the high bits when ldelema index is nint (d…
Browse files Browse the repository at this point in the history 
…otnet#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
  • Loading branch information
@BrzVlad
BrzVlad authored Sep 8, 2023
1 parent 23e81d9 commit 840e8fa
Show file tree
Hide file tree
Showing 9 changed files with 54 additions and 54 deletions.
14 changes: 11 additions & 3 deletions src/mono/mono/mini/decompose.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1545,16 +1545,24 @@ mono_decompose_array_access_opts (MonoCompile *cfg)
ins->inst_imm, ins->flags);
MONO_ADD_INS (cfg->cbb, dest);
break;
case OP_BOUNDS_CHECK:
case OP_BOUNDS_CHECK: {
gboolean need_sext = ins->backend.need_sext;
MONO_EMIT_NULL_CHECK (cfg, ins->sreg1, FALSE);
if (COMPILE_LLVM (cfg)) {
int index2_reg = alloc_preg (cfg);
MONO_EMIT_NEW_UNALU (cfg, OP_SEXT_I4, index2_reg, ins->sreg2);
int index2_reg;
if (need_sext) {
index2_reg = alloc_preg (cfg);
MONO_EMIT_NEW_UNALU (cfg, OP_SEXT_I4, index2_reg, ins->sreg2);
} else {
index2_reg = ins->sreg2;
}
MONO_EMIT_DEFAULT_BOUNDS_CHECK (cfg, ins->sreg1, GINT32_TO_UINT32(ins->inst_imm), index2_reg, ins->flags & MONO_INST_FAULT, ins->inst_p0);
} else {
g_assert (!need_sext);
MONO_ARCH_EMIT_BOUNDS_CHECK (cfg, ins->sreg1, ins->inst_imm, ins->sreg2, ins->inst_p0);
}
break;
}
case OP_NEWARR: {
ERROR_DECL (vt_error);
MonoClass *array_class = mono_class_create_array (ins->inst_newa_class, 1);
Expand Down
21 changes: 0 additions & 21 deletions src/mono/mono/mini/interp/interp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1211,27 +1211,6 @@ ves_array_calculate_index (MonoArray *ao, stackval *sp, gboolean safe)
return pos;
}

static MonoException*
ves_array_get (InterpFrame *frame, stackval *sp, stackval *retval, MonoMethodSignature *sig, gboolean safe)
{
MonoObject *o = sp->data.o;
MonoArray *ao = (MonoArray *) o;
MonoClass *ac = o->vtable->klass;

g_assert (m_class_get_rank (ac) >= 1);

gint32 pos = ves_array_calculate_index (ao, sp + 1, safe);
if (pos == -1)
return mono_get_exception_index_out_of_range ();

gint32 esize = mono_array_element_size (ac);
gconstpointer ea = mono_array_addr_with_size_fast (ao, esize, pos);

MonoType *mt = sig->ret;
stackval_from_data (mt, retval, ea, FALSE);
return NULL;
}

static MonoException*
ves_array_element_address (InterpFrame *frame, MonoClass *required_type, MonoArray *ao, gpointer *ret, stackval *sp, gboolean needs_typecheck)
{
Expand Down
7 changes: 4 additions & 3 deletions src/mono/mono/mini/intrinsics.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,9 +356,10 @@ emit_span_intrinsics (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSignature
/* Similar to mini_emit_ldelema_1_ins () */
int size = mono_class_array_element_size (param_class);

int index_reg = mini_emit_sext_index_reg (cfg, args [1]);
gboolean need_sext;
int index_reg = mini_emit_sext_index_reg (cfg, args [1], &need_sext);

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

// FIXME: Sign extend index ?

Expand Down Expand Up @@ -849,7 +850,7 @@ mini_emit_inst_for_method (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSign
#else
index_reg = args [1]->dreg;
#endif
MONO_EMIT_BOUNDS_CHECK (cfg, args [0]->dreg, MonoString, length, index_reg);
MONO_EMIT_BOUNDS_CHECK (cfg, args [0]->dreg, MonoString, length, index_reg, FALSE);

#if defined(TARGET_X86) || defined(TARGET_AMD64)
EMIT_NEW_X86_LEA (cfg, ins, args [0]->dreg, index_reg, 1, MONO_STRUCT_OFFSET (MonoString, chars));
Expand Down
8 changes: 5 additions & 3 deletions src/mono/mono/mini/ir-emit.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -971,11 +971,12 @@ static int ccount = 0;
#endif

static inline void
mini_emit_bounds_check_offset (MonoCompile *cfg, int array_reg, int array_length_offset, int index_reg, const char *ex_name)
mini_emit_bounds_check_offset (MonoCompile *cfg, int array_reg, int array_length_offset, int index_reg, const char *ex_name, gboolean need_sext)
{
if (!(cfg->opt & MONO_OPT_UNSAFE)) {
ex_name = ex_name ? ex_name : "IndexOutOfRangeException";
if (!(cfg->opt & MONO_OPT_ABCREM)) {
g_assert (!need_sext);
MONO_EMIT_NULL_CHECK (cfg, array_reg, FALSE);
if (COMPILE_LLVM (cfg))
MONO_EMIT_DEFAULT_BOUNDS_CHECK ((cfg), (array_reg), GINT_TO_UINT(array_length_offset), (index_reg), TRUE, ex_name);
Expand All @@ -988,6 +989,7 @@ mini_emit_bounds_check_offset (MonoCompile *cfg, int array_reg, int array_length
ins->sreg2 = index_reg;
ins->inst_p0 = (gpointer)ex_name;
ins->inst_imm = (array_length_offset);
ins->backend.need_sext = need_sext;
ins->flags |= MONO_INST_FAULT;
MONO_ADD_INS ((cfg)->cbb, ins);
(cfg)->flags |= MONO_CFG_NEEDS_DECOMPOSE;
Expand All @@ -1002,8 +1004,8 @@ mini_emit_bounds_check_offset (MonoCompile *cfg, int array_reg, int array_length
* array_length_field is the field in the previous struct with the length
* index_reg is the vreg holding the index
*/
#define MONO_EMIT_BOUNDS_CHECK(cfg, array_reg, array_type, array_length_field, index_reg) do { \
mini_emit_bounds_check_offset ((cfg), (array_reg), MONO_STRUCT_OFFSET (array_type, array_length_field), (index_reg), NULL); \
#define MONO_EMIT_BOUNDS_CHECK(cfg, array_reg, array_type, array_length_field, index_reg, need_sext) do { \
mini_emit_bounds_check_offset ((cfg), (array_reg), MONO_STRUCT_OFFSET (array_type, array_length_field), (index_reg), NULL, need_sext); \
} while (0)

#endif
33 changes: 26 additions & 7 deletions src/mono/mono/mini/method-to-ir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4211,20 +4211,27 @@ mini_field_access_needs_cctor_run (MonoCompile *cfg, MonoMethod *method, MonoCla
}

int
mini_emit_sext_index_reg (MonoCompile *cfg, MonoInst *index)
mini_emit_sext_index_reg (MonoCompile *cfg, MonoInst *index, gboolean *need_sext)
{
int index_reg = index->dreg;
int index2_reg;

*need_sext = FALSE;

#if SIZEOF_REGISTER == 8
// If index is not I4 don't sign extend otherwise we lose high word
if (index->type != STACK_I4)
return index_reg;

/* The array reg is 64 bits but the index reg is only 32 */
if (COMPILE_LLVM (cfg)) {
if (cfg->opt & MONO_OPT_ABCREM) {
/*
* abcrem can't handle the OP_SEXT_I4, so add this after abcrem,
* during OP_BOUNDS_CHECK decomposition, and in the implementation
* of OP_X86_LEA for llvm.
*/
index2_reg = index_reg;
*need_sext = TRUE;
} else {
index2_reg = alloc_preg (cfg);
MONO_EMIT_NEW_UNALU (cfg, OP_SEXT_I4, index2_reg, index_reg);
Expand Down Expand Up @@ -4259,7 +4266,9 @@ mini_emit_ldelema_1_ins (MonoCompile *cfg, MonoClass *klass, MonoInst *arr, Mono
mult_reg = alloc_preg (cfg);
array_reg = arr->dreg;

realidx2_reg = index2_reg = mini_emit_sext_index_reg (cfg, index);
gboolean need_sext;

realidx2_reg = index2_reg = mini_emit_sext_index_reg (cfg, index, &need_sext);

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

if (bcheck)
MONO_EMIT_BOUNDS_CHECK (cfg, array_reg, MonoArray, max_length, realidx2_reg);
MONO_EMIT_BOUNDS_CHECK (cfg, array_reg, MonoArray, max_length, realidx2_reg, need_sext);

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

MonoInst *index_ins = sp [1];
#if SIZEOF_REGISTER == 8
if (sp [1]->type == STACK_I4) {
// stelemref wrapper receives index as native int, sign extend it
guint32 dreg = alloc_preg (cfg);
guint32 sreg = index_ins->dreg;
EMIT_NEW_UNALU (cfg, index_ins, OP_SEXT_I4, dreg, sreg);
}
#endif

iargs [2] = sp [2];
iargs [1] = sp [1];
iargs [1] = index_ins;
iargs [0] = sp [0];

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

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

MONO_EMIT_BOUNDS_CHECK (cfg, array_reg, MonoArray, max_length, index_reg);
MONO_EMIT_BOUNDS_CHECK (cfg, array_reg, MonoArray, max_length, index_reg, FALSE);
EMIT_NEW_LOAD_MEMBASE_TYPE (cfg, ins, m_class_get_byval_arg (klass), array_reg, (target_mgreg_t)offset);
} else {
addr = mini_emit_ldelema_1_ins (cfg, klass, sp [0], sp [1], TRUE, FALSE);
Expand Down
10 changes: 0 additions & 10 deletions src/mono/mono/mini/mini-amd64.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -480,16 +480,6 @@ typedef struct {
/* Used for optimization, not complete */
#define MONO_ARCH_IS_OP_MEMBASE(opcode) ((opcode) == OP_X86_PUSH_MEMBASE)

#define MONO_ARCH_EMIT_BOUNDS_CHECK(cfg, array_reg, offset, index_reg, ex_name) do { \
MonoInst *inst; \
MONO_INST_NEW ((cfg), inst, OP_AMD64_ICOMPARE_MEMBASE_REG); \
inst->inst_basereg = array_reg; \
inst->inst_offset = offset; \
inst->sreg2 = index_reg; \
MONO_ADD_INS ((cfg)->cbb, inst); \
MONO_EMIT_NEW_COND_EXC (cfg, LE_UN, ex_name); \
} while (0)

// Does the ABI have a volatile non-parameter register, so tailcall
// can pass context to generics or interfaces?
#define MONO_ARCH_HAVE_VOLATILE_NON_PARAM_REGISTER 1
Expand Down
3 changes: 2 additions & 1 deletion src/mono/mono/mini/mini.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,7 @@ struct MonoInst {
gpointer data;
gint shift_amount;
gboolean is_pinvoke; /* for variables in the unmanaged marshal format */
gboolean need_sext; /* for OP_BOUNDS_CHECK */
gboolean record_cast_details; /* For CEE_CASTCLASS */
MonoInst *spill_var; /* for OP_MOVE_I4_TO_F/F_TO_I4 and OP_FCONV_TO_R8_X */
guint16 source_opcode; /*OP_XCONV_R8_TO_I4 needs to know which op was used to do proper widening*/
Expand Down Expand Up @@ -2316,7 +2317,7 @@ void mini_emit_memset (MonoCompile *cfg, int destreg, int offset, i
void mini_emit_stobj (MonoCompile *cfg, MonoInst *dest, MonoInst *src, MonoClass *klass, gboolean native);
void mini_emit_initobj (MonoCompile *cfg, MonoInst *dest, const guchar *ip, MonoClass *klass);
void mini_emit_init_rvar (MonoCompile *cfg, int dreg, MonoType *rtype);
int mini_emit_sext_index_reg (MonoCompile *cfg, MonoInst *index);
int mini_emit_sext_index_reg (MonoCompile *cfg, MonoInst *index, gboolean *need_sext);
MonoInst* mini_emit_ldelema_1_ins (MonoCompile *cfg, MonoClass *klass, MonoInst *arr, MonoInst *index, gboolean bcheck, gboolean bounded);
MonoInst* mini_emit_get_gsharedvt_info_klass (MonoCompile *cfg, MonoClass *klass, MonoRgctxInfoType rgctx_type);
MonoInst* mini_emit_get_rgctx_method (MonoCompile *cfg, int context_used,
Expand Down
6 changes: 3 additions & 3 deletions src/mono/mono/mini/simd-intrinsics.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3240,12 +3240,12 @@ emit_sys_numerics_vector_t (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSig
}

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

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

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

/* CopyTo () does complicated argument checks */
mini_emit_bounds_check_offset (cfg, array_ins->dreg, MONO_STRUCT_OFFSET (MonoArray, max_length), index_ins->dreg, "ArgumentOutOfRangeException");
mini_emit_bounds_check_offset (cfg, array_ins->dreg, MONO_STRUCT_OFFSET (MonoArray, max_length), index_ins->dreg, "ArgumentOutOfRangeException", FALSE);
end_index_reg = alloc_ireg (cfg);
int len_reg = alloc_ireg (cfg);
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);
Expand Down
6 changes: 3 additions & 3 deletions src/tests/issues.targets
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1204,9 +1204,6 @@
<ExcludeList Include="$(XunitTestBinBase)/Interop/PInvoke/Int128/Int128TestFieldLayout/*">
<Issue>https://github.com/dotnet/runtime/issues/74223</Issue>
</ExcludeList>
<ExcludeList Include = "$(XUnitTestBinBase)/JIT/Directed/Arrays/nintindexoutofrange/**">
<Issue>https://github.com/dotnet/runtime/issues/71656</Issue>
</ExcludeList>
<ExcludeList Include = "$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/Sse2.X64/StoreNonTemporal_r/**">
<Issue>https://github.com/dotnet/runtime/issues/54176</Issue>
</ExcludeList>
Expand Down Expand Up @@ -2234,6 +2231,9 @@
<ExcludeList Include = "$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_74635/Runtime_74635_1/**">
<Issue>https://github.com/dotnet/runtime/issues/74687</Issue>
</ExcludeList>
<ExcludeList Include = "$(XUnitTestBinBase)/JIT/Directed/Arrays/nintindexoutofrange/**">
<Issue>https://github.com/dotnet/runtime/issues/71656</Issue>
</ExcludeList>
<!-- End interpreter issues -->
</ItemGroup>

Expand Down

0 comments on commit 840e8fa

Please sign in to comment.