Re: [Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns.

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

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@tarantool.org>
>> Co-authored-by: Sergey Kaplun <skaplun@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
>