Re: [Tarantool-patches] [PATCH luajit 2/2] Avoid negation of signed integers in C that may hold INT*_MIN.

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

Hi, Maxim!
Thanks for the review!
Fixed your comments, see the iterative patch below.
Branch is force-pushed.

===================================================================
diff --git a/test/tarantool-tests/lj-928-int-min-negation.test.lua b/test/tarantool-tests/lj-928-int-min-negation.test.lua
index 26f4ed8e..982bba73 100644
--- a/test/tarantool-tests/lj-928-int-min-negation.test.lua
+++ b/test/tarantool-tests/lj-928-int-min-negation.test.lua
@@ -12,7 +12,7 @@ local INT32_MIN = -0x80000000
 local INT64_MIN = -0x8000000000000000
 local TOBIT_CHAR_MAX = 254
 
--- XXX: Many tests (`tonumber()`-related) are failed under UBSan
+-- XXX: Many tests (`tonumber()`-related) are failing under UBSan
 -- with DUALNUM enabled. They are included to avoid regressions in
 -- the future if such a build becomes the default.
 local ffi = require('ffi')
@@ -23,7 +23,7 @@ test:plan(14)
 jit.opt.start('hotloop=1')
 
 -- Temporary variable for the results.
-local r
+local result
 
 -- <src/lj_vmmath.c>:`lj_vm_modi()`
 for _ = 1, 4 do
@@ -31,16 +31,16 @@ for _ = 1, 4 do
   -- Operands should be constants on the trace.
   local x = -0x80000000
   local y = -0x80000000
-  r = x % y
+  result = x % y
 end
-test:is(r, 0, 'no UB during lj_vm_modi')
+test:is(result, 0, 'no UB during lj_vm_modi')
 
 -- <src/lj_strfmt.c>:`lj_strfmt_wint()`
 for _ = 1, 4 do
-  -- Operand should be the constant on the trace.
-  r = tostring(bit.tobit(0x80000000))
+  -- Operand should be a constant on the trace.
+  result = tostring(bit.tobit(0x80000000))
 end
-test:is(r, '-2147483648', 'no UB during lj_strfmt_wint')
+test:is(result, '-2147483648', 'no UB during lj_strfmt_wint')
 
 -- <src/lj_strfmt.c>:`lj_strfmt_putfxint()`
 test:is(('%d'):format(INT64_MIN), '-9223372036854775808',
@@ -70,17 +70,17 @@ test:is(bit.tohex(0, INT32_MIN), TOHEX_EXPECTED, 'no UB during bit_tohex')
 -- The second argument is the number of bytes to be represented.
 -- The negative value stands for uppercase.
 for _ = 1, 4 do
-  -- The second argument should be the constant on the trace.
-  r = bit.tohex(0, -0x80000000)
+  -- The second argument should be a constant on the trace.
+  result = bit.tohex(0, -0x80000000)
 end
-test:is(r, TOHEX_EXPECTED, 'no UB during recording bit.tohex')
+test:is(result, TOHEX_EXPECTED, 'no UB during recording bit.tohex')
 
 -- <src/lj_opt_fold.c>:`simplify_intsub_k()`
-r = 0
+result = 0
 for _ = 1, 4 do
-  r = r - 0x8000000000000000LL
+  result = result - 0x8000000000000000LL
 end
-test:is(r, 0LL, 'no UB during simplify_intsub_k')
+test:is(result, 0LL, 'no UB during simplify_intsub_k')
 
 -- <src/lj_strscan.c>:`strscan_hex()`
 test:is(tonumber('-0x80000000'), INT32_MIN, 'no UB during strscan_hex')
===================================================================

On 01.07.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits below.
> 
> On Tue, Jun 25, 2024 at 06:54:25PM GMT, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >

<snipped>

> > diff --git a/test/tarantool-tests/lj-928-int-min-negation.test.lua b/test/tarantool-tests/lj-928-int-min-negation.test.lua
> > new file mode 100644
> > index 00000000..26f4ed8e
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-928-int-min-negation.test.lua
> > @@ -0,0 +1,121 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate LuaJIT's UBSan failures during
> > +-- `INT*_MIN` negation.
> > +-- See also: https://github.com/LuaJIT/LuaJIT/issues/928.
> > +
> > +local test = tap.test('lj-928-int-min-negation.'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +local INT32_MIN = -0x80000000
> > +local INT64_MIN = -0x8000000000000000
> > +local TOBIT_CHAR_MAX = 254
> > +
> > +-- XXX: Many tests (`tonumber()`-related) are failed under UBSan
> Typo: s/are failed/are failing/

Fixed, thanks!

> > +-- with DUALNUM enabled. They are included to avoid regressions in
> > +-- the future if such a build becomes the default.
> > +local ffi = require('ffi')
> > +local LL_T = ffi.typeof(1LL)
> > +
> > +test:plan(14)
> > +
> > +jit.opt.start('hotloop=1')
> > +
> > +-- Temporary variable for the results.
> > +local r
> Let's name it `result` or `tmp_result` then.

Renamed.

> 
> > +
> > +-- <src/lj_vmmath.c>:`lj_vm_modi()`
> > +for _ = 1, 4 do
> > +  -- Use additional variables to avoid folding during parsing.
> > +  -- Operands should be constants on the trace.
> > +  local x = -0x80000000
> > +  local y = -0x80000000
> > +  r = x % y
> > +end
> > +test:is(r, 0, 'no UB during lj_vm_modi')
> > +
> > +-- <src/lj_strfmt.c>:`lj_strfmt_wint()`
> > +for _ = 1, 4 do
> > +  -- Operand should be the constant on the trace.
> Typo: s/the constant/a constant/

Fixed.

> > +  r = tostring(bit.tobit(0x80000000))

<snipped>

> > --
> > 2.45.1
> >

-- 
Best regards,
Sergey Kaplun