From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id AE0D3652501; Wed, 4 Oct 2023 15:52:00 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org AE0D3652501 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1696423920; bh=/iaKi9rFwqyjU03CvqN21rRfJbyjyUETXbCX5vEnsYk=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=hnvrlHaYK5JiervFegbfu0+V3QsoOb4EkoJHe8woWL3gHqtBnbYd3f5t+gansR12P tcOu8/QcSNdUdWM90FjgXumsj4Y1+NJjZ8/snlSfAbAgmdLOPv3eAW0d0dsCeIxYIZ sfM/EVTtEDqQp/feelD3jY8rX3NDVOqhj6zbT4Tk= Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id BBFBC6522BC for ; Wed, 4 Oct 2023 15:50:47 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BBFBC6522BC Received: by mail-lf1-f49.google.com with SMTP id 2adb3069b0e04-5046bf37ec1so2554072e87.1 for ; Wed, 04 Oct 2023 05:50:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696423847; x=1697028647; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=5KparVgqELQCvGFsHZbuiNw1qY9R1Z7fOAWAz9xWZvk=; b=W4WSIa9E9LQIDjlc/+1jweUtlEHYFK6bjLsq3kdMCraH8hnIduSNygxNF3C92b77cC 8lf7Rty7d178Zyt2amw4WTe0eHGxVbwuRVpXbzt5EQIZiU7xc/gGKd9wvqMk4zcZ8q9X oID7v2kXSpYTZ45Rl5gek7glbc2GrSIDj2DQL04kQwm3TzgntUE/KnH1aUJoz5Ws9jRI 8S1ypjee/gsaDQ/iA87lLdmRsqe1A+2t9oDQuKv95NfDJvkibykH6eORdgt4oVu+KI0E dNwC9qCQSroD3FQTSNhx0/9Jp54HRg3iyMZEYa0DsZyBBT91P/Vejg9zT/1WYgMi2lus vR8g== X-Gm-Message-State: AOJu0YzrOYXG1zFknCtxrLL3F3TvSZ6YcfewIhMprddcRiWe4IsEqGVw NSyCJb4PKsawoE058tPpTdPFEl2mVneQvA== X-Google-Smtp-Source: AGHT+IF3MbTmD0evCLtrg/KnaYauF6XTE+EBkvp0RKH5kUyVePgHtW56aV6TPYTaBSNDphIdm951PQ== X-Received: by 2002:a05:6512:3582:b0:500:99a9:bc40 with SMTP id m2-20020a056512358200b0050099a9bc40mr1778473lfr.69.1696423846589; Wed, 04 Oct 2023 05:50:46 -0700 (PDT) Received: from localhost.localdomain ([185.205.79.32]) by smtp.gmail.com with ESMTPSA id d1-20020ac24c81000000b0050326307edesm596379lfl.125.2023.10.04.05.50.45 (version=TLS1_3 cipher=TLS_CHACHA20_POLY1305_SHA256 bits=256/256); Wed, 04 Oct 2023 05:50:46 -0700 (PDT) To: tarantool-patches@dev.tarantool.org, sergeyb@tarantool.org, skaplun@tarantool.org, m.kokryashkin@tarantool.org Date: Wed, 4 Oct 2023 15:50:34 +0300 Message-Id: <20231004125034.64110-3-max.kokryashkin@gmail.com> X-Mailer: git-send-email 2.39.3 (Apple Git-145) In-Reply-To: <20231004125034.64110-1-max.kokryashkin@gmail.com> References: <20231004125034.64110-1-max.kokryashkin@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH luajit v3 2/2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maksim Kokryashkin via Tarantool-patches Reply-To: Maksim Kokryashkin Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" From: Mike Pall Reported by Arseny Vakhrushev. Fix contributed by Peter Cawley. (cherry-picked from commit 5c46f47736f7609be407c88d531ecd1689d40a79) As specified in the comment in `lj_record_stop`, all loops must set `J->pc` to the next instruction. However, the chunk of logic in `lj_trace_exit` expects it to be set to `BC_JLOOP` itself if it used to be a `BC_RET`. This wrong pc results in the execution of random data that goes after `BC_JLOOP` in the case of restoration from the snapshot. This patch fixes that behavior by adapting the loop recording logic to this specific case. NOTICE: This patch is only a part of the original commit, and the other part is backported in the previous commit. The patch was split into two, so the test case becomes easier to implement since it can now depend on this assertion instead of memory layout. Maxim Kokryashkin: * added the description and the test for the problem Part of tarantool/tarantool#9145 --- src/lj_record.c | 9 +- .../lj-624-jloop-snapshot-pc.test.lua | 84 +++++++++++++++++++ 2 files changed, 89 insertions(+), 4 deletions(-) create mode 100644 test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua diff --git a/src/lj_record.c b/src/lj_record.c index 48a5481b..3bdc6134 100644 --- a/src/lj_record.c +++ b/src/lj_record.c @@ -570,10 +570,10 @@ static LoopEvent rec_iterl(jit_State *J, const BCIns iterins) } /* Record LOOP/JLOOP. Now, that was easy. */ -static LoopEvent rec_loop(jit_State *J, BCReg ra) +static LoopEvent rec_loop(jit_State *J, BCReg ra, int skip) { if (ra < J->maxslot) J->maxslot = ra; - J->pc++; + J->pc += skip; return LOOPEV_ENTER; } @@ -2433,7 +2433,7 @@ void lj_record_ins(jit_State *J) rec_loop_interp(J, pc, rec_iterl(J, *pc)); break; case BC_LOOP: - rec_loop_interp(J, pc, rec_loop(J, ra)); + rec_loop_interp(J, pc, rec_loop(J, ra, 1)); break; case BC_JFORL: @@ -2443,7 +2443,8 @@ void lj_record_ins(jit_State *J) rec_loop_jit(J, rc, rec_iterl(J, traceref(J, rc)->startins)); break; case BC_JLOOP: - rec_loop_jit(J, rc, rec_loop(J, ra)); + rec_loop_jit(J, rc, rec_loop(J, ra, + !bc_isret(bc_op(traceref(J, rc)->startins)))); break; case BC_IFORL: diff --git a/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua new file mode 100644 index 00000000..726b2efa --- /dev/null +++ b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua @@ -0,0 +1,84 @@ +local tap = require('tap') +local test = tap.test('lj-624-jloop-snapshot-pc'):skipcond({ + ['Test requires JIT enabled'] = not jit.status(), +}) + +test:plan(1) +-- XXX: The test case below triggers the assertion that was +-- added in the patch if tested without the fix itself. It +-- is hard to create a stable reproducer without turning off +-- ASLR and VM randomizations, which is not suitable for testing. + +-- Reproducer below produces the following traces: +-- ---- TRACE 1 start test.lua:2 +-- 0001 KSHORT 1 2 +-- 0002 ISGE 0 1 +-- 0003 JMP 1 => 0006 +-- 0006 UGET 1 0 ; fib +-- 0007 SUBVN 2 0 0 ; 1 +-- 0008 CALL 1 2 2 +-- 0000 . FUNCF 4 ; test.lua:2 +-- 0001 . KSHORT 1 2 +-- 0002 . ISGE 0 1 +-- 0003 . JMP 1 => 0006 +-- 0006 . UGET 1 0 ; fib +-- 0007 . SUBVN 2 0 0 ; 1 +-- 0008 . CALL 1 2 2 +-- 0000 . . FUNCF 4 ; test.lua:2 +-- 0001 . . KSHORT 1 2 +-- 0002 . . ISGE 0 1 +-- 0003 . . JMP 1 => 0006 +-- 0006 . . UGET 1 0 ; fib +-- 0007 . . SUBVN 2 0 0 ; 1 +-- 0008 . . CALL 1 2 2 +-- 0000 . . . FUNCF 4 ; test.lua:2 +-- ---- TRACE 1 stop -> up-recursion +-- +-- ---- TRACE 1 exit 1 +-- ---- TRACE 2 start 1/1 test.lua:3 +-- 0004 ISTC 1 0 +-- 0005 JMP 1 => 0013 +-- 0013 RET1 1 2 +-- 0009 UGET 2 0 ; fib +-- 0010 SUBVN 3 0 1 ; 2 +-- 0011 CALL 2 2 2 +-- 0000 . JFUNCF 4 1 ; test.lua:2 +-- ---- TRACE 2 stop -> 1 +-- +-- ---- TRACE 2 exit 1 +-- ---- TRACE 3 start 2/1 test.lua:3 +-- 0013 RET1 1 2 +-- 0012 ADDVV 1 1 2 +-- 0013 RET1 1 2 +-- ---- TRACE 3 abort test.lua:3 -- down-recursion, restarting +-- +-- ---- TRACE 3 start test.lua:3 +-- 0013 RET1 1 2 +-- 0009 UGET 2 0 ; fib +-- 0010 SUBVN 3 0 1 ; 2 +-- 0011 CALL 2 2 2 +-- 0000 . JFUNCF 4 1 ; test.lua:2 +-- ---- TRACE 3 stop -> 1 +-- +-- ---- TRACE 2 exit 1 +-- ---- TRACE 4 start 2/1 test.lua:3 +-- 0013 RET1 1 2 +-- 0012 ADDVV 1 1 2 +-- 0013 JLOOP 3 3 +-- +-- During the recording of the latter JLOOP the assertion added +-- in the patch is triggered. +-- +-- See also: +-- https://github.com/luaJIT/LuaJIT/issues/624 + + +jit.opt.start('hotloop=1', 'hotexit=1') +local function fib(n) + return n < 2 and n or fib(n - 1) + fib(n - 2) +end + +fib(5) + +test:ok(true, 'snapshot pc is correct') +test:done(true) -- 2.39.3 (Apple Git-145)