From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: Sergey Bronnikov <estetus@gmail.com>, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns. Date: Thu, 13 Jul 2023 12:55:51 +0300 [thread overview] Message-ID: <77ec03d9-546e-f04b-a711-cf8a979bd2c6@tarantool.org> (raw) In-Reply-To: <ZK+uh1MCYhhKwEOA@root> Hi, Sergey! On 7/13/23 10:57, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the fixes! > > Still some thoughts about the `pcall()`. > > On 10.07.23, Sergey Bronnikov wrote: >> Hi, Sergey! <snipped> >> @@ -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. > Yes, but there is no trace related to the `f()` only for > `test:check()`: > > | ---- TRACE 1 start tap.lua:33 > | ---- TRACE 2 start 1/stitch tap.lua:34 > | ---- TRACE 3 start tap.lua:16 > | ---- TRACE 3 start tap.lua:80 > > So, with this `pcall()` we lose the JIT testing. With patch below JIT assertion is triggered: --- a/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua +++ b/test/tarantool-tests/lj-819-fix-missing-uclo.test.lua @@ -101,10 +101,10 @@ local function missing_uclo() end local f = missing_uclo() -local res = f() +local res_interp = f() -- Without a patch, we don't get here a function, because upvalue -- isn't closed as desirable. -test:ok(type(res) == 'function', +test:ok(type(res_interp) == 'function', 'VM consistency: type of returned value is correct') jit.opt.start('hotloop=1') @@ -112,9 +112,8 @@ jit.opt.start('hotloop=1') f = missing_uclo() f() f = missing_uclo() -local _ -_, res = pcall(f) -test:ok(type(res) == 'function', +local _, res_jit = pcall(f) +test:ok(type(res_jit) == 'function', 'consistency on compilation: type of returned value is correct') os.exit(test:check() and 0 or 1) In a 1:1 conversation we agreed that it is ok. I added patch to the branch and force-pushed.
next prev parent reply other threads:[~2023-07-13 9:55 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-05-30 16:56 Sergey Bronnikov via Tarantool-patches 2023-06-06 12:51 ` Sergey Kaplun via Tarantool-patches 2023-06-07 11:35 ` Maxim Kokryashkin via Tarantool-patches 2023-07-06 9:43 ` Sergey Bronnikov via Tarantool-patches 2023-07-06 11:31 ` Maxim Kokryashkin via Tarantool-patches 2023-07-06 13:45 ` Sergey Bronnikov via Tarantool-patches 2023-07-06 21:12 ` Maxim Kokryashkin via Tarantool-patches 2023-07-06 9:40 ` Sergey Bronnikov via Tarantool-patches 2023-07-09 13:15 ` Sergey Kaplun via Tarantool-patches 2023-07-10 14:53 ` Sergey Bronnikov via Tarantool-patches 2023-07-13 7:57 ` Sergey Kaplun via Tarantool-patches 2023-07-13 9:55 ` Sergey Bronnikov via Tarantool-patches [this message] 2023-07-13 10:25 ` Sergey Kaplun via Tarantool-patches 2023-07-20 18:37 ` Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=77ec03d9-546e-f04b-a711-cf8a979bd2c6@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=estetus@gmail.com \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox