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 3FC5350F07E; Thu, 6 Jul 2023 16:45:25 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3FC5350F07E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1688651125; bh=ftiQXaAKLj6vg4StNWrgvMwLP9b9PDZ1Cbg7FzjtRR8=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=JacNvNVXHyYPD078dtIpHQ8/cmkF5c3urMqVi6qj33dGrdzaPEiXLlL82kW3Sef5P 7+6NweqHH/6deXOjYItQCR8W+B92Pqj1QjS9V+DAHhQreyw19fBrLfibS3rAQ/R9JL 4GyBl6fZlSYuSEuOJ4Q4mHOiZ1xCRz8UQxV1/f2M= Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [95.163.41.73]) (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 C6A7A50F07E for ; Thu, 6 Jul 2023 16:45:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C6A7A50F07E Received: by smtp32.i.mail.ru with esmtpa (envelope-from ) id 1qHPIQ-00Brmp-G8; Thu, 06 Jul 2023 16:45:23 +0300 Content-Type: multipart/alternative; boundary="------------Z5Jh6ZVEHT0FJEE8gDFcYNdQ" Message-ID: <06ef0ded-e52e-eae2-1046-72cda475f2ae@tarantool.org> Date: Thu, 6 Jul 2023 16:45:21 +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 References: <1686137725.949044142@f485.i.mail.ru> <1688643085.266702017@f729.i.mail.ru> In-Reply-To: <1688643085.266702017@f729.i.mail.ru> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD91BCCBDE3AB822A0AEC5C16A2844EE78305DF504B46E4AAA9182A05F538085040A79747E25BB37F16C05B898CF2CBD429B91E21E1517D32CCD8C8BB0553AA4ECE X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE771540F9ECFC94C4BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C83F54BD885518138638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D80FF4CD641E9826254CCA1E8897FE9654117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BC0EC8C44E4C1BEE2A471835C12D1D977C4224003CC836476EB9C41850244470149FD398EE364050F9647ADFADE5905B16136E347CC761E07C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947C5B63D382EEF4D8962E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89F83C798A30B85E16BA91E23F1B6B78B78B5C8C57E37DE458BEDA766A37F9254B7 X-B7AD71C0: 6FEFE4C63DFE2D85718F43753FD9AD21390936EE2A242988633761F9DFDC81E9FAEB67242C3A2DCAFD6066BCC4120689 X-C1DE0DAB: 0D63561A33F958A5C2CA5DDC9BD9BAD17AD7B761A864C418F3D08B7E949CBC7CF87CCE6106E1FC07E67D4AC08A07B9B0DB8A315C1FF4794DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC71106E36FF2641B7B8424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D347130F804358653A6B83677C7033EB7BA0E34CE07666F7161443E28EDA630E07FADB6EE08B240A1B71D7E09C32AA3244CC734772ECE4420FC738A0E165535D6B83C6EB905E3A8056BBAD658CF5C8AB4025DA084F8E80FEBD3FFA33E6B6B2F82C47A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojtT0gG7HlXRJo48OhvXeZVw== X-Mailru-Sender: 0A26D9779F8DDEAB68A88147A9E30CE8EAEBC218F44A91A8516950B20C38C0027984EDE3151E855D645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 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 Cc: Sergey Bronnikov , tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------Z5Jh6ZVEHT0FJEE8gDFcYNdQ Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Test requires jit and it failed on jobs without a JIT Fixed! On 7/6/23 14:31, Maxim Kokryashkin wrote: > Hi! > Thanks for the fixes! > A few CI jobs are red, please address them. > -- > Best regards, > Maxim Kokryashkin > > 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 >> --------------Z5Jh6ZVEHT0FJEE8gDFcYNdQ Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Test requires jit and it failed on jobs without a JIT

Fixed!

On 7/6/23 14:31, Maxim Kokryashkin wrote:
Hi!
Thanks for the fixes!
A few CI jobs are red, please address them.
--
Best regards,
Maxim Kokryashkin
 
 
 

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
 
 
--------------Z5Jh6ZVEHT0FJEE8gDFcYNdQ--