[Tarantool-patches] [PATCH luajit 2/2] Avoid negation of signed integers in C that may hold INT*_MIN.
Sergey Kaplun
skaplun at tarantool.org
Mon Jul 1 13:12:29 MSK 2024
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
More information about the Tarantool-patches
mailing list