Skip to content

Commit

Permalink
Fix negative division in JIT
Browse files Browse the repository at this point in the history
Also fix divide-by-zero in mod_i, since mod_i
is implemented by signed division anyway (and
n % 0 has no mathematical sense).
  • Loading branch information
bdw committed Aug 27, 2014
1 parent a10dc26 commit 5b574f8
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 16 deletions.
9 changes: 7 additions & 2 deletions src/core/interp.c
Expand Up @@ -351,10 +351,15 @@ void MVM_interp_run(MVMThreadContext *tc, void (*initial_invoke)(MVMThreadContex
GET_REG(cur_op, 0).ui64 = GET_REG(cur_op, 2).ui64 / GET_REG(cur_op, 4).ui64;
cur_op += 6;
goto NEXT;
OP(mod_i):
GET_REG(cur_op, 0).i64 = GET_REG(cur_op, 2).i64 % GET_REG(cur_op, 4).i64;
OP(mod_i): {
MVMint64 numer = GET_REG(cur_op, 2).i64;
MVMint64 denom = GET_REG(cur_op, 4).i64;
if (denom == 0)
MVM_exception_throw_adhoc(tc, "Modulation by zero");
GET_REG(cur_op, 0).i64 = numer % denom;
cur_op += 6;
goto NEXT;
}
OP(mod_u):
GET_REG(cur_op, 0).ui64 = GET_REG(cur_op, 2).ui64 % GET_REG(cur_op, 4).ui64;
cur_op += 6;
Expand Down
43 changes: 29 additions & 14 deletions src/jit/emit_x64.dasc
Expand Up @@ -606,8 +606,6 @@ void MVM_jit_emit_primitive(MVMThreadContext *tc, MVMJitGraph *jg,
case MVM_OP_add_i:
case MVM_OP_sub_i:
case MVM_OP_mul_i:
case MVM_OP_div_i:
case MVM_OP_mod_i:
case MVM_OP_bor_i:
case MVM_OP_band_i:
case MVM_OP_bxor_i: {
Expand All @@ -625,13 +623,6 @@ void MVM_jit_emit_primitive(MVMThreadContext *tc, MVMJitGraph *jg,
case MVM_OP_mul_i:
| imul rax, WORK[reg_c];
break;
case MVM_OP_div_i:
case MVM_OP_mod_i:
// Convert Quadword to Octoword, i.e. use rax:rdx as one
// single 16 byte register
| cqo;
| idiv qword WORK[reg_c];
break;
case MVM_OP_bor_i:
| or rax, WORK[reg_c];
break;
Expand All @@ -642,15 +633,39 @@ void MVM_jit_emit_primitive(MVMThreadContext *tc, MVMJitGraph *jg,
| xor rax, WORK[reg_c];
break;
}
if (ins->info->opcode == MVM_OP_mod_i) {
// result of modula is stored in rdx
| mov WORK[reg_a], rdx;
| mov WORK[reg_a], rax;
break;
}
case MVM_OP_div_i:
case MVM_OP_mod_i: {
MVMint16 dst = ins->operands[0].reg.orig;
MVMint16 a = ins->operands[1].reg.orig;
MVMint16 b = ins->operands[2].reg.orig;
| mov rcx, WORK[b];
| test rcx, rcx;
| jnz >1;
| throw_adhoc "Division by zero";
|1:
// division is safe, proceed
| mov rax, WORK[a];
| cqo; // convert quadword to octoword. look it up :-)
| idiv rcx; // rcx was the denominator
if (op == MVM_OP_div_i) {
| mov r8, rax;
| dec r8;
| cmp rax, 0; // quotient < 0
| setl ch;
| test rdx, rdx; // remainder != 0
| setnz cl;
| test ch, cl;
| cmovnz rax, r8;
| mov WORK[dst], rax;
} else {
// all others in rax
| mov WORK[reg_a], rax;
| mov WORK[dst], rdx;
}
break;
}

case MVM_OP_inc_i: {
MVMint32 reg = ins->operands[0].reg.orig;
| inc qword WORK[reg];
Expand Down

2 comments on commit 5b574f8

@zhuomingliang
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello @bdw, I would suggest reverting this commit, as per rakudo/rakudo@98c32aaa8b and Raku/old-design-docs@ed80b33d5e .

@bdw
Copy link
Contributor Author

@bdw bdw commented on 5b574f8 Aug 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I can be excused in first reading up on the preceeding discussion. Most of this commit is actually about rounding-towards-negative-infinity of integer division as is done in MoarVM. If you really wish integer division to cause a hardware level exception, note that (currently) it will not be caught, it will just bring down your VM. Thanks for bringing it to my attention.

Please sign in to comment.