Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mono][jit] Don't truncate the high bits when ldelema index is nint #88986

Merged
merged 5 commits into from
Sep 8, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/mono/mono/mini/abcremoval.c
Original file line number Diff line number Diff line change
Expand Up @@ -1144,7 +1144,7 @@ process_block (MonoCompile *cfg, MonoBasicBlock *bb, MonoVariableRelationsEvalua
if (TRACE_ABC_REMOVAL)
mono_print_ins (ins);

if (ins->opcode == OP_BOUNDS_CHECK) { /* Handle OP_LDELEMA2D, too */
if (ins->opcode == OP_BOUNDS_CHECK || ins->opcode == OP_BOUNDS_CHECK_SEXT) { /* Handle OP_LDELEMA2D, too */
array_var = ins->sreg1;
index_var = ins->sreg2;

Expand Down Expand Up @@ -1204,6 +1204,7 @@ MONO_DISABLE_WARNING(4127) /* conditional expression is constant */
*/
if (COMPILE_LLVM (cfg) && (ins->opcode == OP_LDLEN ||
ins->opcode == OP_BOUNDS_CHECK ||
ins->opcode == OP_BOUNDS_CHECK_SEXT ||
ins->opcode == OP_STRLEN ||
(MONO_IS_LOAD_MEMBASE (ins) && (ins->flags & MONO_INST_FAULT)) ||
(MONO_IS_STORE_MEMBASE (ins) && (ins->flags & MONO_INST_FAULT)))) {
Expand Down
13 changes: 11 additions & 2 deletions src/mono/mono/mini/decompose.c
Original file line number Diff line number Diff line change
Expand Up @@ -1546,15 +1546,24 @@ mono_decompose_array_access_opts (MonoCompile *cfg)
MONO_ADD_INS (cfg->cbb, dest);
break;
case OP_BOUNDS_CHECK:
case OP_BOUNDS_CHECK_SEXT: {
gboolean need_sext = ins->opcode == OP_BOUNDS_CHECK_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
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
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
12 changes: 8 additions & 4 deletions src/mono/mono/mini/ir-emit.h
Original file line number Diff line number Diff line change
Expand Up @@ -971,19 +971,23 @@ 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);
else
MONO_ARCH_EMIT_BOUNDS_CHECK ((cfg), (array_reg), GINT_TO_UINT(array_length_offset), (index_reg), ex_name);
} else {
MonoInst *ins;
MONO_INST_NEW ((cfg), ins, OP_BOUNDS_CHECK);
if (need_sext)
MONO_INST_NEW ((cfg), ins, OP_BOUNDS_CHECK_SEXT);
else
MONO_INST_NEW ((cfg), ins, OP_BOUNDS_CHECK);
ins->sreg1 = array_reg;
ins->sreg2 = index_reg;
ins->inst_p0 = (gpointer)ex_name;
Expand All @@ -1002,8 +1006,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
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 recevies index as native int, sign extend it
guint32 dreg = alloc_preg (cfg);
vargaz marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -10588,7 +10607,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
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
1 change: 1 addition & 0 deletions src/mono/mono/mini/mini-ops.h
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,7 @@ MINI_OP(OP_NEWARR, "newarr", IREG, IREG, NONE)
MINI_OP(OP_LDLEN, "ldlen", IREG, IREG, NONE)
/* inst_p0 is the exception name to throw or NULL */
MINI_OP(OP_BOUNDS_CHECK, "bounds_check", NONE, IREG, IREG)
MINI_OP(OP_BOUNDS_CHECK_SEXT, "bounds_check_sext", NONE, IREG, IREG)
vargaz marked this conversation as resolved.
Show resolved Hide resolved
/* type checks */
MINI_OP(OP_ISINST, "isinst", IREG, IREG, NONE)
MINI_OP(OP_CASTCLASS, "castclass", IREG, IREG, NONE)
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/mini/mini.h
Original file line number Diff line number Diff line change
Expand Up @@ -2316,7 +2316,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
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
Original file line number Diff line number Diff line change
Expand Up @@ -1203,9 +1203,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 @@ -2233,6 +2230,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
Loading