From 8ee50b9adddd2d149ed355402cfbcdf45f9d4f38 Mon Sep 17 00:00:00 2001 From: Mike Pall Date: Sun, 13 Mar 2022 18:32:32 +0100 Subject: [PATCH] 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 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 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 NOTE: After emitting the bytecode instruction BC_FNEW fixup is not required, because FuncState will set a flag PROTO_CHILD that will trigger emitting a pair of instructions BC_UCLO and BC_RET (see ) 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. (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 Part of tarantool/tarantool#8825 Signed-off-by: Sergey Bronnikov --- src/lj_parse.c | 2 +- .../lj-819-fix-missing-uclo.test.lua | 115 ++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) create mode 100644 test/tarantool-tests/lj-819-fix-missing-uclo.test.lua diff --git a/src/lj_parse.c b/src/lj_parse.c index af0dc53fa6..343fa797c3 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 0000000000..942c22b2a4 --- /dev/null +++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua @@ -0,0 +1,115 @@ +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 +-- generated for a function missing_uclo() with and without a patch. +-- Both listings contains two BC_UCLO instructions: +-- - first one with id 0004 is generated for a statement 'break' inside +-- condition, see label BC_UCLO1; +-- - second one with id 0009 is generated for a statement 'return' inside +-- 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 +-- a function prototype, meets first BC_UCLO instruction (break) and forgives a +-- second one (return). This leads to a wrong result produced by a function +-- 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 +-- fixup BC_UCLO and problem does not appear. +-- +-- Listing 1. Bytecode with 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 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 +-- 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. + -- 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 +-- as desirable. +test:ok(type(res) == 'function', 'virtual machine consistency: type of returned value is correct') + +-- Make JIT compiler aggressive. +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') + +os.exit(test:check() and 0 or 1)