Tarantool development patches archive
 help / color / mirror / Atom feed
From: Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: tarantool-patches@dev.tarantool.org, imun@tarantool.org,
	skaplun@tarantool.org
Subject: [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies.
Date: Wed, 30 Mar 2022 02:48:54 +0300	[thread overview]
Message-ID: <f06569bf4ab2263b1a5e26dc704939d33c64f84e.1648597663.git.m.kokryashkin@tarantool.org> (raw)
In-Reply-To: <cover.1648597663.git.m.kokryashkin@tarantool.org>

From: Mike Pall <mike>

math.min()/math.max() could produce different results when compiled.
Previously, these functions did not check for the number of arguments,
which in some situations led to incorrect behavior.

Consider this example:
```
jit.opt.start(0, "hotloop=1")

local r, msg = pcall(function () math.min() end)
r, msg = pcall(function () math.min() end)
```
Before this patch, this snippet of code completed successfully, although it
was not supposed to, since there were no args to math.min().

This patch adds check for the number of arguments for jit-compiled
version of math.min/max, and it also adds the corresponding test case for
the mentioned issue.

Part of tarantool/tarantool#6163
Part of tarantool/tarantool#6548
---
 src/lj_asm_arm.h                              |  6 +--
 src/lj_asm_arm64.h                            |  6 +--
 src/lj_opt_fold.c                             | 53 +++++++------------
 src/lj_vmmath.c                               |  4 +-
 src/vm_arm.dasc                               |  4 +-
 src/vm_arm64.dasc                             |  4 +-
 src/vm_x64.dasc                               |  2 +-
 src/vm_x86.dasc                               |  2 +-
 .../gh-6163-jit-min-max.test.lua              | 20 +++++++
 9 files changed, 53 insertions(+), 48 deletions(-)
 create mode 100644 test/tarantool-tests/gh-6163-jit-min-max.test.lua

diff --git a/src/lj_asm_arm.h b/src/lj_asm_arm.h
index 4fd08b9e..84ab06c0 100644
--- a/src/lj_asm_arm.h
+++ b/src/lj_asm_arm.h
@@ -1664,8 +1664,8 @@ static void asm_min_max(ASMState *as, IRIns *ir, int cc, int fcc)
     asm_intmin_max(as, ir, cc);
 }
 
-#define asm_min(as, ir)		asm_min_max(as, ir, CC_GT, CC_HI)
-#define asm_max(as, ir)		asm_min_max(as, ir, CC_LT, CC_LO)
+#define asm_min(as, ir)		asm_min_max(as, ir, CC_GT, CC_PL)
+#define asm_max(as, ir)		asm_min_max(as, ir, CC_LT, CC_LE)
 
 /* -- Comparisons --------------------------------------------------------- */
 
@@ -1857,7 +1857,7 @@ static void asm_hiop(ASMState *as, IRIns *ir)
   } else if ((ir-1)->o == IR_MIN || (ir-1)->o == IR_MAX) {
     as->curins--;  /* Always skip the loword min/max. */
     if (uselo || usehi)
-      asm_sfpmin_max(as, ir-1, (ir-1)->o == IR_MIN ? CC_HI : CC_LO);
+      asm_sfpmin_max(as, ir-1, (ir-1)->o == IR_MIN ? CC_PL : CC_LE);
     return;
 #elif LJ_HASFFI
   } else if ((ir-1)->o == IR_CONV) {
diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h
index cc8c0c9d..e93e29e3 100644
--- a/src/lj_asm_arm64.h
+++ b/src/lj_asm_arm64.h
@@ -1594,7 +1594,7 @@ static void asm_fpmin_max(ASMState *as, IRIns *ir, A64CC fcc)
   Reg dest = (ra_dest(as, ir, RSET_FPR) & 31);
   Reg right, left = ra_alloc2(as, ir, RSET_FPR);
   right = ((left >> 8) & 31); left &= 31;
-  emit_dnm(as, A64I_FCSELd | A64F_CC(fcc), dest, left, right);
+  emit_dnm(as, A64I_FCSELd | A64F_CC(fcc), dest, right, left);
   emit_nm(as, A64I_FCMPd, left, right);
 }
 
@@ -1606,8 +1606,8 @@ static void asm_min_max(ASMState *as, IRIns *ir, A64CC cc, A64CC fcc)
     asm_intmin_max(as, ir, cc);
 }
 
-#define asm_max(as, ir)		asm_min_max(as, ir, CC_GT, CC_HI)
-#define asm_min(as, ir)		asm_min_max(as, ir, CC_LT, CC_LO)
+#define asm_min(as, ir)		asm_min_max(as, ir, CC_LT, CC_PL)
+#define asm_max(as, ir)		asm_min_max(as, ir, CC_GT, CC_LE)
 
 /* -- Comparisons --------------------------------------------------------- */
 
diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
index 3c508062..29b760b4 100644
--- a/src/lj_opt_fold.c
+++ b/src/lj_opt_fold.c
@@ -1774,8 +1774,6 @@ LJFOLDF(reassoc_intarith_k64)
 #endif
 }
 
-LJFOLD(MIN MIN any)
-LJFOLD(MAX MAX any)
 LJFOLD(BAND BAND any)
 LJFOLD(BOR BOR any)
 LJFOLDF(reassoc_dup)
@@ -1785,6 +1783,15 @@ LJFOLDF(reassoc_dup)
   return NEXTFOLD;
 }
 
+LJFOLD(MIN MIN any)
+LJFOLD(MAX MAX any)
+LJFOLDF(reassoc_dup_minmax)
+{
+  if (fins->op2 == fleft->op2)
+    return LEFTFOLD;  /* (a o b) o b ==> a o b */
+  return NEXTFOLD;
+}
+
 LJFOLD(BXOR BXOR any)
 LJFOLDF(reassoc_bxor)
 {
@@ -1823,23 +1830,12 @@ LJFOLDF(reassoc_shift)
   return NEXTFOLD;
 }
 
-LJFOLD(MIN MIN KNUM)
-LJFOLD(MAX MAX KNUM)
 LJFOLD(MIN MIN KINT)
 LJFOLD(MAX MAX KINT)
 LJFOLDF(reassoc_minmax_k)
 {
   IRIns *irk = IR(fleft->op2);
-  if (irk->o == IR_KNUM) {
-    lua_Number a = ir_knum(irk)->n;
-    lua_Number y = lj_vm_foldarith(a, knumright, fins->o - IR_ADD);
-    if (a == y)  /* (x o k1) o k2 ==> x o k1, if (k1 o k2) == k1. */
-      return LEFTFOLD;
-    PHIBARRIER(fleft);
-    fins->op1 = fleft->op1;
-    fins->op2 = (IRRef1)lj_ir_knum(J, y);
-    return RETRYFOLD;  /* (x o k1) o k2 ==> x o (k1 o k2) */
-  } else if (irk->o == IR_KINT) {
+  if (irk->o == IR_KINT) {
     int32_t a = irk->i;
     int32_t y = kfold_intop(a, fright->i, fins->o);
     if (a == y)  /* (x o k1) o k2 ==> x o k1, if (k1 o k2) == k1. */
@@ -1852,24 +1848,6 @@ LJFOLDF(reassoc_minmax_k)
   return NEXTFOLD;
 }
 
-LJFOLD(MIN MAX any)
-LJFOLD(MAX MIN any)
-LJFOLDF(reassoc_minmax_left)
-{
-  if (fins->op2 == fleft->op1 || fins->op2 == fleft->op2)
-    return RIGHTFOLD;  /* (b o1 a) o2 b ==> b; (a o1 b) o2 b ==> b */
-  return NEXTFOLD;
-}
-
-LJFOLD(MIN any MAX)
-LJFOLD(MAX any MIN)
-LJFOLDF(reassoc_minmax_right)
-{
-  if (fins->op1 == fright->op1 || fins->op1 == fright->op2)
-    return LEFTFOLD;  /* a o2 (a o1 b) ==> a; a o2 (b o1 a) ==> a */
-  return NEXTFOLD;
-}
-
 /* -- Array bounds check elimination -------------------------------------- */
 
 /* Eliminate ABC across PHIs to handle t[i-1] forwarding case.
@@ -1995,8 +1973,6 @@ LJFOLDF(comm_comp)
 
 LJFOLD(BAND any any)
 LJFOLD(BOR any any)
-LJFOLD(MIN any any)
-LJFOLD(MAX any any)
 LJFOLDF(comm_dup)
 {
   if (fins->op1 == fins->op2)  /* x o x ==> x */
@@ -2004,6 +1980,15 @@ LJFOLDF(comm_dup)
   return fold_comm_swap(J);
 }
 
+LJFOLD(MIN any any)
+LJFOLD(MAX any any)
+LJFOLDF(comm_dup_minmax)
+{
+  if (fins->op1 == fins->op2)  /* x o x ==> x */
+    return LEFTFOLD;
+  return NEXTFOLD;
+}
+
 LJFOLD(BXOR any any)
 LJFOLDF(comm_bxor)
 {
diff --git a/src/lj_vmmath.c b/src/lj_vmmath.c
index b231d3e8..08ccf467 100644
--- a/src/lj_vmmath.c
+++ b/src/lj_vmmath.c
@@ -50,8 +50,8 @@ double lj_vm_foldarith(double x, double y, int op)
 #if LJ_HASJIT
   case IR_ATAN2 - IR_ADD: return atan2(x, y); break;
   case IR_LDEXP - IR_ADD: return ldexp(x, (int)y); break;
-  case IR_MIN - IR_ADD: return x > y ? y : x; break;
-  case IR_MAX - IR_ADD: return x < y ? y : x; break;
+  case IR_MIN - IR_ADD: return x < y ? x : y; break;
+  case IR_MAX - IR_ADD: return x > y ? x : y; break;
 #endif
   default: return x;
   }
diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc
index 21f7fecb..6b511347 100644
--- a/src/vm_arm.dasc
+++ b/src/vm_arm.dasc
@@ -1718,8 +1718,8 @@ static void build_subroutines(BuildCtx *ctx)
   |.endif
   |.endmacro
   |
-  |  math_minmax math_min, gt, hi
-  |  math_minmax math_max, lt, lo
+  |  math_minmax math_min, gt, pl
+  |  math_minmax math_max, lt, le
   |
   |//-- String library -----------------------------------------------------
   |
diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
index 313cc94f..80e795ae 100644
--- a/src/vm_arm64.dasc
+++ b/src/vm_arm64.dasc
@@ -1491,8 +1491,8 @@ static void build_subroutines(BuildCtx *ctx)
   |  b <6
   |.endmacro
   |
-  |  math_minmax math_min, gt, hi
-  |  math_minmax math_max, lt, lo
+  |  math_minmax math_min, gt, pl
+  |  math_minmax math_max, lt, le
   |
   |//-- String library -----------------------------------------------------
   |
diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
index 974047d3..cbf5fb9b 100644
--- a/src/vm_x64.dasc
+++ b/src/vm_x64.dasc
@@ -1874,7 +1874,7 @@ static void build_subroutines(BuildCtx *ctx)
   |  jmp ->fff_res
   |
   |.macro math_minmax, name, cmovop, sseop
-  |  .ffunc name
+  |  .ffunc_1 name
   |  mov RAd, 2
   |.if DUALNUM
   |  mov RB, [BASE]
diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
index ab8e6f27..b839eff0 100644
--- a/src/vm_x86.dasc
+++ b/src/vm_x86.dasc
@@ -2292,7 +2292,7 @@ static void build_subroutines(BuildCtx *ctx)
   |  xorps xmm4, xmm4; jmp <1			// Return +-Inf and +-0.
   |
   |.macro math_minmax, name, cmovop, sseop
-  |  .ffunc name
+  |  .ffunc_1 name
   |  mov RA, 2
   |  cmp dword [BASE+4], LJ_TISNUM
   |.if DUALNUM
diff --git a/test/tarantool-tests/gh-6163-jit-min-max.test.lua b/test/tarantool-tests/gh-6163-jit-min-max.test.lua
new file mode 100644
index 00000000..d6eb3f3b
--- /dev/null
+++ b/test/tarantool-tests/gh-6163-jit-min-max.test.lua
@@ -0,0 +1,20 @@
+local tap = require('tap')
+jit.off()
+jit.flush()
+
+local test = tap.test("gh-6163-jit-min-max")
+test:plan(2)
+--
+-- gh-6163: math.min/math.max success with no args
+--
+local pcall = pcall
+
+jit.opt.start(0, 'hotloop=1')
+jit.on()
+
+local r, _ = pcall(function() math.min() end)
+test:ok(false == r)
+r, _ = pcall(function() math.min() end)
+test:ok(false == r)
+
+os.exit(test:check() and 0 or 1)
-- 
2.35.1


  reply	other threads:[~2022-03-29 23:49 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-29 23:48 [Tarantool-patches] [PATCH luajit v4 0/3] fix math.min/max inconsistencies Maxim Kokryashkin via Tarantool-patches
2022-03-29 23:48 ` Maxim Kokryashkin via Tarantool-patches [this message]
2022-04-14  8:55   ` [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies Sergey Kaplun via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 2/3] Don't compile math.modf() anymore Maxim Kokryashkin via Tarantool-patches
2022-04-14  8:53   ` Sergey Kaplun via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 3/3] Cleanup math function compilation and fix inconsistencies Maxim Kokryashkin via Tarantool-patches
2022-04-14  8:53   ` Sergey Kaplun via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f06569bf4ab2263b1a5e26dc704939d33c64f84e.1648597663.git.m.kokryashkin@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox