[Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns.
Sergey Bronnikov
sergeyb at tarantool.org
Mon Jul 10 17:53:39 MSK 2023
Hi, Sergey!
thanks for review!
On 7/9/23 16:15, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the fixes!
> LGTM, except a few nits and rewordings below.
>
>> Fix BC_UCLO insertion for returns.
>>
>> Contributed by XmiliaH.
>>
>> (cherry-picked from commit 93a65d3cc263aef2d2feb3d7ff2206aca3bee17e)
>>
>> Patch fixes a problem when LuaJIT generates a wrong bytecode with a
>> missed BC_UCLO instruction. When some of BC_RET bytecode instructions are
> Nit: commit line length is more than 72 symbols (see rationale here [1]).
Fixed!
>
>> not fixup-ed, due to an early return, if UCLO is obtained before, those
>> leads to VM inconsistency after return from the function.
>>
>> Patch makes the following changes in bytecode (thats it, emits extra
> Typo: s/thats/that's/
> Nit: I suggest to drop introductory phrase "thats it".
Dropped.
>
>> BC_UCLO instruction that closes upvalues):
>>
>> @@ -11,11 +11,12 @@
>> 0006 => LOOP 1 => 0012
>> 0007 ISF 0
>> 0008 JMP 1 => 0010
>> -0009 RET1 0 2
>> +0009 UCLO 0 => 0014
>> 0010 => FNEW 0 0 ; uclo.lua:56
>> 0011 JMP 1 => 0006
>> 0012 => UCLO 0 => 0001
>> 0013 => RET0 0 1
>> +0014 => RET1 0 2
> Side note: like this diff very much, good idea!
>
>> NOTE: After emitting the bytecode instruction BC_FNEW fixup is not
> Typo: s/NOTE: //
Dropped.
>
>> required, because FuncState will set a flag PROTO_CHILD that will
>> trigger emitting a pair of instructions BC_UCLO and BC_RET (see
>> <src/lj_parse.c:2355>) and BC_RET will close all upvalues from a base
>> equal to 0.
>>
>> JIT compilation of missing_uclo() function without a patch with fix is failed:
>> src/lj_record.c:135: rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) - (TRef)(IRT_NUM) <= (TRef)(IRT_INT-IRT_NUM)))' failed.
> I suppose there is no need to cite this assertion failure. So, I suggest
> to reword this paragraph in the following way:
>
> | JIT compilation of `missing_uclo()` function without a patch leads to
> | assertion failure in `rec_check_slots()` due to Lua stack inconsistency.
> | The root cause is still parsing misbehaviour. Nevertheless, the test
> | case is added to be sure that the JIT compilation isn't affected.
>
> Also, there is no need to mention me in the commit message :).
Replaced with suggested paragraph. Thanks.
>
>> (Thanks to Sergey Kaplun for discovering this!)
>> Thus second testcase in a test covers a case with compilation as well.
>>
>> Sergey Bronnikov:
>> * added the description and the test for the problem
>>
>> Signed-off-by: Sergey Bronnikov <sergeyb at tarantool.org>
>> Co-authored-by: Sergey Kaplun <skaplun at tarantool.org>
>>
>> diff --git a/src/lj_parse.c b/src/lj_parse.c
>> index af0dc53f..343fa797 100644
>> --- a/src/lj_parse.c
>> +++ b/src/lj_parse.c
>> @@ -1546,7 +1546,7 @@ static void fs_fixup_ret(FuncState *fs)
>> /* Replace with UCLO plus branch. */
>> fs->bcbase[pc].ins = BCINS_AD(BC_UCLO, 0, offset);
>> break;
>> - case BC_UCLO:
>> + case BC_FNEW:
>> return; /* We're done. */
>> default:
>> break;
>> diff --git a/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>> new file mode 100644
>> index 00000000..942c22b2
>> --- /dev/null
>> +++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
>> @@ -0,0 +1,115 @@
> Please, restrict comment length to the 66 symbols in the test.
Updated.
+++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
@@ -1,31 +1,33 @@
local tap = require('tap')
--- Test contains a reproducer for a problem when LuaJIT generates a wrong
--- bytecode with a missed BC_UCLO instruction.
+-- Test contains a reproducer for a problem when LuaJIT generates
+-- a wrong bytecode with a missed BC_UCLO instruction.
local test = tap.test('lj-819-fix-missing-uclo'):skipcond({
['Test requires JIT enabled'] = not jit.status(),
})
>> +local tap = require('tap')
>> +-- Test contains a reproducer for a problem when LuaJIT generates a wrong
>> +-- bytecode with a missed BC_UCLO instruction.
>> +local test = tap.test('lj-819-fix-missing-uclo'):skipcond({
>> + ['Test requires JIT enabled'] = not jit.status(),
>> +})
>> +
>> +test:plan(2)
>> +
>> +-- Let's take a look at listings Listing 1 and Listing 2 below with bytecode
> Typo: s/bytecode/the bytecode/
Fixed.
>
>> +-- generated for a function missing_uclo() with and without a patch.
> Minor: I suggest to use `` to line-up that this is functions names in
> the code. Feel free to ignore.
Updated.
>
>> +-- Both listings contains two BC_UCLO instructions:
> Typo: s/contains/contain/
Updated.
>
>> +-- - first one with id 0004 is generated for a statement 'break' inside
> Typo: s/first/the first/
Fixed.
>
>> +-- condition, see label BC_UCLO1;
>> +-- - second one with id 0009 is generated for a statement 'return' inside
> Typo: s/second/the second/
Fixed.
>
>> +-- a nested loop, see label BC_UCLO2;
>> +-- Both BC_UCLO's closes upvalues after leaving a function's scope.
>> +--
>> +-- The problem is happen when fs_fixup_ret() traverses bytecode instructions in
> Typo: s/happen/happened/
Fixed.
>
>> +-- a function prototype, meets first BC_UCLO instruction (break) and forgives a
> Typo: s/first/the first/
Fixed.
>
>> +-- second one (return). This leads to a wrong result produced by a function
> Nit: I suggest the reword this in the following way:
> | and misses the second one return to fixup
>
>> +-- returned by missing_uclo() function. This also explains why do we need a
>> +-- dead code in reproducer - without first BC_UCLO fs_fixup_ret() successfully
> Typo: s/first/the first/
Fixed.
>
>> +-- fixup BC_UCLO and problem does not appear.
> Typo: s/problem/the problem/
Fixed.
>
>> +--
>> +-- Listing 1. Bytecode with a fix.
>> +--
>> +-- -- BYTECODE -- uclo.lua:1-59
> I suggest to use listing from the test itself, i.e. use
> lj-819-fix-missing-uclo.test.lua:79-97 (line numbers are used before
> comment rewording and reallignment) here and below in headers.
>
>> +-- 0001 => LOOP 0 => 0013
>> +-- 0002 JMP 0 => 0003
>> +-- 0003 => JMP 0 => 0005
>> +-- 0004 UCLO 0 => 0013
>> +-- 0005 => KPRI 0 0
>> +-- 0006 => LOOP 1 => 0012
>> +-- 0007 ISF 0
>> +-- 0008 JMP 1 => 0010
>> +-- 0009 UCLO 0 => 0014
>> +-- 0010 => FNEW 0 0 ; uclo.lua:54
> And lj-819-fix-missing-uclo.test.lua:92 here and below in listings.
Fixed here and below.
>
>> +-- 0011 JMP 1 => 0006
>> +-- 0012 => UCLO 0 => 0001
>> +-- 0013 => RET0 0 1
>> +-- 0014 => RET1 0 2
>> +--
>> +-- Listing 2. Bytecode without a fix.
>> +--
>> +-- BYTECODE -- uclo.lua:1-59
>> +-- 0001 => LOOP 0 => 0013
>> +-- 0002 JMP 0 => 0003
>> +-- 0003 => JMP 0 => 0005
>> +-- 0004 UCLO 0 => 0013
>> +-- 0005 => KPRI 0 0
>> +-- 0006 => LOOP 1 => 0012
>> +-- 0007 ISF 0
>> +-- 0008 JMP 1 => 0010
>> +-- 0009 UCLO 0 => 0014
>> +-- 0010 => FNEW 0 0 ; uclo.lua:54
>> +-- 0011 JMP 1 => 0006
>> +-- 0012 => UCLO 0 => 0001
>> +-- 0013 => RET0 0 1
>> +-- 0014 => RET1 0 2
>> +--
>> +-- Listing 3. Changes in bytecode before and after a fix.
>> +--
>> +-- @@ -11,11 +11,12 @@
>> +-- 0006 => LOOP 1 => 0012
>> +-- 0007 ISF 0
>> +-- 0008 JMP 1 => 0010
>> +-- -0009 RET1 0 2
>> +-- +0009 UCLO 0 => 0014
>> +-- 0010 => FNEW 0 0 ; uclo.lua:56
>> +-- 0011 JMP 1 => 0006
>> +-- 0012 => UCLO 0 => 0001
>> +-- 0013 => RET0 0 1
>> +-- +0014 => RET1 0 2
>> +--
>> +-- First testcase checks a correct bytecode generation by frontend
> Typo: s/First/The first/
> Typo: s/frontend/frontend,/
Fixed.
>> +-- and the second testcase checks consistency on a JIT compilation.
>> +
>> +local function missing_uclo()
>> + while true do -- luacheck: ignore
>> + -- Attention: it is not a dead code, it is a part of reproducer.
> Typo: s/reproducer/the reproducer/
Fixed.
>
> I suggest the following rewording:
> | XXX: it is not a dead code, it is a part of the reproducer, see the
> | comment above.
Replaced.
>
>> + -- label: BC_UCLO1
>> + if false then
>> + break
>> + end
>> + local f
>> + while true do
>> + if f then
>> + -- label: BC_UCLO2
>> + return f
>> + end
>> + f = function()
>> + return f
>> + end
>> + end
>> + end
>> +end
>> +
>> +local f = missing_uclo()
>> +local res = f()
>> +-- Without a patch we don't get here a function, because upvalue isn't closed
> Typo: s/Without a patch/Without a patch,/
Fixed.
>
>> +-- as desirable.
>> +test:ok(type(res) == 'function', 'virtual machine consistency: type of returned value is correct')
> Code width is more than 80 symbols.
Fixed.
@@ -98,11 +102,11 @@ end
local f = missing_uclo()
local res = f()
--- Without a patch we don't get here a function, because upvalue isn't
closed
--- as desirable.
-test:ok(type(res) == 'function', 'virtual machine consistency: type of
returned value is correct')
+-- Without a patch, we don't get here a function, because upvalue
+-- isn't closed as desirable.
+test:ok(type(res) == 'function',
+ 'VM consistency: type of returned value is correct')
> Minor: s/virtual machine/VM/. Feel free to ignore.
Fixed.
>
>> +
>> +-- Make JIT compiler aggressive.
> I suggest to drop this comment (there is no need to comment what
> `hotloop=1` do, but if you want you may explain why we use 1 here (this
> is still common practice in our tests, so I suggest just drop the
> comment)).
Dropped.
>> +jit.opt.start('hotloop=1')
>> +
>> +f = missing_uclo()
>> +f()
>> +f = missing_uclo()
>> +local _
>> +_, res = pcall(f)
>> +test:ok(type(res) == 'function', 'consistency on compilation: type of returned value is correct')
> Code width is more than 80 symbols.
Fixed.
@@ -110,6 +114,7 @@ f()
f = missing_uclo()
local _
_, res = pcall(f)
-test:ok(type(res) == 'function', 'consistency on compilation: type of
returned value is correct')
+test:ok(type(res) == 'function',
+ 'consistency on compilation: type of returned value is correct')
os.exit(test:check() and 0 or 1)
> Do we need pcall here?
I would use it to avoid breaking test due to assert.
Without a pcall:
TAP version 13
1..2
not ok - VM consistency: type of returned value is correct
filename: eval
line: -1
frame #1
line: 0
source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
what: main
namewhat:
src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
frame #2
line: -1
source: =[C]
filename: eval
what: C
namewhat:
src: [C]
luajit:
/home/sergeyb/sources/MRG/tarantool/third_party/luajit/src/lj_record.c:135:
rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) -
(TRef)(IRT_NUM) <= (TRef)(IRT_INT-IRT_NUM)))' failed.
Aborted (core dumped)
With pcall:
TAP version 13
1..2
not ok - VM consistency: type of returned value is correct
filename: eval
line: -1
frame #1
line: 0
source: @test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
filename: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
what: main
namewhat:
src: test/tarantool-tests/lj-819-fix-missing-uclo.test.lua
frame #2
line: -1
source: =[C]
filename: eval
what: C
namewhat:
src: [C]
not ok - consistency on compilation: type of returned value is correct
filename: eval
line: -1
frame #1
line: 0
<snipped>
I like second output more.
>
> Also, the test isn't failed with assertion failure as declared. But the
> following one is:
> | LUA_PATH="src/?.lua;;" src/luajit -Ohotloop=1 -e '
> |
> | local function missing_uclo()
> | while true do -- luacheck: ignore
> | local f
> | if false then break end
> | while true do
> | if f then
> | return f
> | end
> | f = function()
> | return f
> | end
> | end
> | end
> | end
> |
> | -- Function to pollute Lua stack.
> | local function ret_arg(f) return f end
> |
> | f = missing_uclo()
> | ret_arg(f())
> | ret_arg(f())
> | '
>
>> +
>> +os.exit(test:check() and 0 or 1)
> [1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message
>
More information about the Tarantool-patches
mailing list