[Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies.
skaplun at tarantool.org
Thu Apr 14 11:55:11 MSK 2022
Thanks for the patch!
Please, consider my comments below.
On 30.03.22, Maxim Kokryashkin wrote:
> 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,
Nit: Didn't check number of arguments for x86/x64 architecture.
> which in some situations led to incorrect behavior.
Nit: I suppose "in some situatitons" may be omitted (these situations is
incorrect (i.e. 0 -- we can mention that) amount of arguments).
> 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().
Nit: I suggest to drop this snippet as far as you allready describe the
behaviour in such case.
> This patch adds check for the number of arguments for jit-compiled
AFAICS, it only adds the corresponding check in the VM (ffunc->ffunc_1), so
there is no incorrect recording as far as we always fall through to ff
> version of math.min/max, and it also adds the corresponding test case for
> the mentioned issue.
We should mention about refactoring, too. I see the following parts:
* removing several associative fold optimizations (we need to check
logic of this optimization, and add the corresponding testcases)
* changing fcc in asm_min_max CC_HI/CC_LO -> CC_PL/CC_PL (maybe you
may investigate this and create a testcase showing "inconsistencies")
> Part of tarantool/tarantool#6163
I tried the reproducer mentioned in this ticket  and its passes with
only this patch. So patches in this patchset may be merged separated
(but in the same order).
> 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
> diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h
> diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc
> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
> 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 @@
I've checked your test on my branch without your patchset and it still
| $ ../../src/luajit gh-6163-jit-min-max.test.lua
| TAP version 13
| ok - nil
| ok - nil
| $ make tarantool-tests
| /home/burii/builds_workspace/luajit/fix-slot-check-for-mm-record/test/tarantool-tests/gh-6163-jit-min-max.test.lua ......................... ok
> +local tap = require('tap')
> +local test = tap.test("gh-6163-jit-min-max")
Nit: Please, use one type of quotes: ''.
> +-- gh-6163: math.min/math.max success with no args
> +local pcall = pcall
> +jit.opt.start(0, 'hotloop=1')
> +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)
More information about the Tarantool-patches