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 9458446B100; Thu, 6 Jul 2023 12:43:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9458446B100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1688636605; bh=cNyjYOqgDoUjvvAvAtVnhY1Dyg+mcpMDwI1OQckkscE=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=kAZDCZlI+8I/5mVKsWTWUqqFyiYRRO8BwJ9yieH5RC4nPvfbiqZZ+74nExBp3EF2X 0f3L6pp4QtUD5gpajujCDei9vYr0XfWiFLXDihr1SEbIG1LZSos9It7RoRjSHyy2H3 5/WMfDWAfv9hjRjCbGS0Tuok55o1pyEBAhtfRYfg= Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [95.163.41.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7559B46B100 for ; Thu, 6 Jul 2023 12:43:24 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7559B46B100 Received: by smtp41.i.mail.ru with esmtpa (envelope-from ) id 1qHLWF-00BFjf-Kt; Thu, 06 Jul 2023 12:43:24 +0300 Content-Type: multipart/alternative; boundary="------------MVDUzewPaJLa74Hg6HWAh0fL" Message-ID: Date: Thu, 6 Jul 2023 12:43:22 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Content-Language: en-US To: Maxim Kokryashkin , Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org, Sergey Bronnikov References: <1686137725.949044142@f485.i.mail.ru> In-Reply-To: <1686137725.949044142@f485.i.mail.ru> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD91BCCBDE3AB822A0A9C6E440A91431E173DD5753A4B11DACF182A05F5380850400F1CC3A4D06BA1DD10F4754E1508075ACE86882F56460BDF18C225E6370166F8 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE77603ADE015AF816DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B97DA3EE4E90F98B8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8CB2F73EAFBFEA54630353BCB2819CB3E117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B990B2EB4F061288CA471835C12D1D977C4224003CC836476EB9C41850244470149FD398EE364050F9647ADFADE5905B16136E347CC761E07C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947C5B63D382EEF4D8962E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F83C798A30B85E16BA91E23F1B6B78B78B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A55E3E9949CF0A366EC5A6F715F2796A43C26229E55F4A188EF87CCE6106E1FC07E67D4AC08A07B9B062B3BD3CC35DA588CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D348BF433665406F3901A1086CAD1D514FF7EC84F3E052CA3D08565C11AC89FF9C9042BFEF8BFBF8DC71D7E09C32AA3244C7345486331706CCF79C1FA377B6ED1A7BBA718C7E6A9E042BAD658CF5C8AB4025DA084F8E80FEBD3FFA33E6B6B2F82C47A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojtT0gG7HlXRKOMJfXcQFK6w== X-Mailru-Sender: 0A26D9779F8DDEAB68A88147A9E30CE83AD770A7D56570077E84A57A301D6A2E219434EAC0284D46645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v1] Fix BC_UCLO insertion for returns. 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: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------MVDUzewPaJLa74Hg6HWAh0fL Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Max! Thanks for review! Added more comments to the test and commit message. New changes force-pushed to the branch. Please take a look. S. On 6/7/23 14:35, Maxim Kokryashkin wrote: > Hi, Sergey and Sergey! > > Hi, Sergey! > Thanks for the patch! > Please, consider my comments below. > > On 30.05.23, Sergey Bronnikov wrote: > > From: Sergey Bronnikov > > > > > Contributed by XmiliaH. > > > > (cherry-picked from commit > 93a65d3cc263aef2d2feb3d7ff2206aca3bee17e) > > > > After emitting bytecode instruction BC_FNEW fixup is not > required, > > Typo: s/bytecode/the bytecode > Fixed, thanks! > > 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 base equal to 0. > > This part describes why replacing UCLO with FNEW is good > enough and > better than just deleting > | case BC_UCLO: return; > But the original problem is that some of BC_RET are not > fixup-ed, due to > early return, if UCLO is obtained before, those leads to VM > inconsistency after return from the function. Please, mention > this too. > > Agree here, it is hard to get what the patch is about from that > description, > without diving into the changes. > Added more details. > > Also, before the patch I got the following assertion in JIT: > > | 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 > | f = missing_uclo() > | print(f()) > | f = missing_uclo() > | print(f()) > | ' > | 3.1002202036551 > | luajit: > /home/burii/reviews/luajit/lj-819-missing-uclo/src/lj_record.c:135: > rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) - > (TRef)(IRT_NUM) <= (TRef) > | (IRT_INT-IRT_NUM)))' failed. > | Aborted > > I don't sure that we should test this particular failure too, > since the > origin of the problem is the incorrect emitted bytecode. > > Thoughts? > > We should not, because it is most likely caused by the issue > that was fixed in the LuaJIT/LuaJIT@5c46f477. > assert in rec_check_slots could be for many reasons, so I added a testcase for compiler too. > > > -- > > 2.34.1 > > > > -- > Best regards, > Sergey Kaplun > > -- > Best regards, > Maxim Kokryashkin > --------------MVDUzewPaJLa74Hg6HWAh0fL Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Max!


Thanks for review! Added more comments to the test and commit message.

New changes force-pushed to the branch. Please take a look.


S.

On 6/7/23 14:35, Maxim Kokryashkin wrote:
Hi, Sergey and Sergey!
 
 
 
Hi, Sergey!
Thanks for the patch!
Please, consider my comments below.

On 30.05.23, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Contributed by XmiliaH.
>
> (cherry-picked from commit 93a65d3cc263aef2d2feb3d7ff2206aca3bee17e)
>
> After emitting bytecode instruction BC_FNEW fixup is not required,
Typo: s/bytecode/the bytecode

Fixed, thanks!


> 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 base equal to 0.

This part describes why replacing UCLO with FNEW is good enough and
better than just deleting
| case BC_UCLO: return;
But the original problem is that some of BC_RET are not fixup-ed, due to
early return, if UCLO is obtained before, those leads to VM
inconsistency after return from the function. Please, mention this too.
Agree here, it is hard to get what the patch is about from that description,
without diving into the changes.

Added more details.


<snipped>
Also, before the patch I got the following assertion in JIT:

| 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
| f = missing_uclo()
| print(f())
| f = missing_uclo()
| print(f())
| '
| 3.1002202036551
| luajit: /home/burii/reviews/luajit/lj-819-missing-uclo/src/lj_record.c:135: rec_check_slots: Assertion `((((((tr))>>24) & IRT_TYPE) - (TRef)(IRT_NUM) <= (TRef)
| (IRT_INT-IRT_NUM)))' failed.
| Aborted

I don't sure that we should test this particular failure too, since the
origin of the problem is the incorrect emitted bytecode.

Thoughts?
We should not, because it is most likely caused by the issue
that was fixed in the LuaJIT/LuaJIT@5c46f477.


assert in rec_check_slots could be for many reasons, so I added a testcase for compiler too.



> --
> 2.34.1
>

--
Best regards,
Sergey Kaplun
--
Best regards,
Maxim Kokryashkin
 
--------------MVDUzewPaJLa74Hg6HWAh0fL--