Re: [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!

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

Does it?
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
fallback.

> 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 [1] 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

<snipped>

> diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h

<snipped>

> diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c

<snipped>

> diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc

<snipped>

> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc

<snipped>

> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc

<snipped>

> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc

<snipped>

> 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
passes:

| $ ../../src/luajit gh-6163-jit-min-max.test.lua
| TAP version 13
| 1..2
| 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')
> +jit.off()
> +jit.flush()
> +
> +local test = tap.test("gh-6163-jit-min-max")

Nit: Please, use one type of quotes: ''.

> +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)

[1]: https://github.com/tarantool/tarantool/issues/6163

--
Best regards,
Sergey Kaplun