[Tarantool-patches] [PATCH luajit] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.
Sergey Kaplun
skaplun at tarantool.org
Fri Sep 22 10:51:04 MSK 2023
Hi, Maxim!
Thanks for the patch!
LGTM, after adding a comment in test with verbose description (see
below).
On 21.09.23, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> Reported by Arseny Vakhrushev.
> Fix contributed by Peter Cawley.
>
> As specified in lj_record.c:304, all loops must set `J->pc` to
Minor: it's better to mention function names than numbers of lines
since they can easily change.
> the next instruction. However, the chunk of logic at
> lj_trace.c:923 expects it to be set to `BC_JLOOP` itself if it
Ditto.
> 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
Typo: s/BC_JLOOP/`BC_JLOOP`/
> restoration from the snapshot.
>
> This patch fixes that behavior by adapting the loop recording
> logic to this specific case.
>
> Maxim Kokryashkin:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#8825
> ---
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-624-jloop-snapshot-pc
> PR: https://github.com/tarantool/tarantool/pull/9166
>
> NB: The test for this patch triggers the assertion added in this patch,
> however I had no luck making a __stable__ reproducer for the issue,
> since it depends on what's in memory after the BC_JLOOP. It is easier to
> achieve a consitent failures if ASLR is disabled, but it's not suitable
> for the testing purposes.
I'm OK with testing it as is.
We may add the comment about the newly added assertion to the test too.
>
> src/lj_record.c | 9 +++++----
> src/lj_snap.c | 3 +++
> .../lj-624-jloop-snapshot-pc.test.lua | 16 ++++++++++++++++
> 3 files changed, 24 insertions(+), 4 deletions(-)
> create mode 100644 test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
>
<snipped>
> 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..ada290ff
> --- /dev/null
> +++ b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
> @@ -0,0 +1,16 @@
> +local tap = require('tap')
> +local test = tap.test('lj-624-jloop-snapshot-pc'):skipcond({
> + ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +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)
AFAICS, the assertion is failed at the moment of the `JLOOP` BC
recording. May you please add descriptions of traces layout and taken
snapshot exits? This helps to understand the test case.
| ---- TRACE 4 start 2/1 lj-624-jloop-snapshot-pc.test.lua:10
| 0013 RET1 1 2
| 0012 ADDVV 1 1 2
| 0013 JLOOP 3 3
> +
> +test:ok(true, 'snapshot pc is correct')
> +test:done(true)
> --
> 2.42.0
>
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list