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 1A6CCDAE66B; Fri, 22 Nov 2024 15:53:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1A6CCDAE66B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1732280035; bh=CS02loa8X3mZGmHjSB8ykIuigq8n2cpUPYGWCl5ipvo=; 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=F/5DYxEwHbu/wpNvAo0I+Ia6H7vuNMp4apOVhCYaZlXqC4+eLun+ARUzh3zYk1t2e q+hZ0XVSpf/ualCPhRh9ogScH3MEiLLgJ9AJ8kCboyIrrdRhdhD8ZOAE1SSgLpMZlh v1m+FSLHMV0kb58YZZbFukg3Mb4hEb75gFfeU724= Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [95.163.41.89]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 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 0CDBB3B29AA for ; Fri, 22 Nov 2024 15:53:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0CDBB3B29AA Received: by exim-smtp-5787fdfb9c-kzwxq with esmtpa (envelope-from ) id 1tETAW-0000000067y-0BAg; Fri, 22 Nov 2024 15:53:52 +0300 Content-Type: multipart/alternative; boundary="------------KYpCsdmOxbbIrerhuUW1kcVm" Message-ID: <19c0c442-68a9-4181-8424-7d3e202289f9@tarantool.org> Date: Fri, 22 Nov 2024 15:53:51 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun , Sergey Bronnikov References: <76c9c163c365aec70741162ab83fdfa6385a9118.1730976041.git.sergeyb@tarantool.org> Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD94DF3B3E5E6F579E3EA7FF382466C99897A5877A8CABCDE1D182A05F5380850408A7FDB060E7C93103DE06ABAFEAF67054AB3C6769E2C3B0EC2F29820CA3437BEE2E35834FA4BCB46 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE73B2A9F8A35432468EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637BB680D3A894950458638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8DE73DFF85F6303222069A4EC8688517BC185ED3CC7B72AE5CC7F00164DA146DAFE8445B8C89999728AA50765F79006370BDB19F53EE528DD389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8744B801E316CB65FF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C5E3BF8C76DC23F746E0066C2D8992A164AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3A56AE056EABF0D1EBA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7924082EF4C287A56731C566533BA786AA5CC5B56E945C8DA X-87b9d050: 1 X-C1DE0DAB: 0D63561A33F958A53AE5E9CA1DAB37765002B1117B3ED696A9958CEC07FFE58A466072E6821086B3823CB91A9FED034534781492E4B8EEAD003C2D46C52F18F2BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34D71B56C992F8AF19DA0C5D45B01157C8A4CDEA5AD98192FC5385FA4F84E1969EFDAD8C9B40913E451D7E09C32AA3244C3D82BA1C03DCD0CB77DD89D51EBB7742EAD4F297FAB8B659EA455F16B58544A21C197AAF4D2E4732A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojB5M2t/ETq/2izvtSuOPa3A== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F25884582D9A6F53163AC54E117A099264967948BECCE552980B82DFFBEB1EEEFE0D026D645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE 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: 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. --------------KYpCsdmOxbbIrerhuUW1kcVm Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Sergey, please see below. Updated version has been force-pushed. Sergey On 12.11.2024 22:23, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the patch! > Nice catch! > > Please, consider my comments below. > > On 07.11.24, Sergey Bronnikov wrote: >> From: Sergey Bronnikov >> >> The patch fixes a problem with recording `getmetatable()` >> for I/O object: recording of `getmetatable` call with a file > Typo: s > Typo: s/recording of/recording the/ > Typo: s/a file/file/ Fixed. > >> descriptors represented by userdata object `UDTYPE_IO_FILE` >> (like `io.stdout`) leads to violation of assertion in >> `rec_check_slots`. > Nit: > | ... by the userdata object `UDTYPE_IO_FILE` (like `io.stdout`) always > | stores `nil` instead of the given metatable. This leads to the > | violation of the assertion in `rec_check_slots`. > >> Note, the problem was fixed upstream in different manner, see >> commit 5141cbc20c43 > Nit: Please, use full commit hash. Fixed, but usually ten characters is enough [1]: > Generally, eight to ten characters are more than enough to be unique within a project. For example, as of February 2019, the Linux kernel (which is a fairly sizable project) has over 875,000 commits and almost seven million objects in its object database, with no two objects whose SHA-1s are identical in the first 12 characters. 1. https://git-scm.com/book/en/v2/Git-Tools-Revision-Selection > Also, please describe that specialized recording (introduced in the > upstream) lacks the check of the metatable precense. So, the fix in > upstream is incomplete (according to the comment [1]). Added:     Note, the problem was fixed upstream in different manner, see     commit 5141cbc20c43921778ff36be541c15888bee8ee3     ("Fix compiliation of getmetatable() for UDTYPE_IO_FILE.").     Note, the specialization omits the check of `__metatable` field     precense and the check for the metatable itself. So, if we change     the metatable on the object after the trace is compiled, the trace     becomes invalid. The proposed test demonstrates these cases as     well. > > Also, please add both testcases from the issue comment [1] (don't forget > to restore metatable after the first of them). Added. >> ("Fix compiliation of getmetatable() for UDTYPE_IO_FILE."). >> --- >> src/lj_record.c | 2 +- >> ...-incorrect-recording-getmetatable.test.lua | 21 +++++++++++++++++++ >> 2 files changed, 22 insertions(+), 1 deletion(-) >> create mode 100644 test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua >> >> diff --git a/src/lj_record.c b/src/lj_record.c >> index cc97bdf9..7181b72a 100644 >> --- a/src/lj_record.c >> +++ b/src/lj_record.c > > >> diff --git a/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua b/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua >> new file mode 100644 >> index 00000000..8bf22ca7 >> --- /dev/null >> +++ b/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua >> @@ -0,0 +1,21 @@ >> +local tap = require('tap') >> + >> +local test = tap.test('lj-1279-incorrect-recording-getmetatable') >> +test:plan(1) >> + >> +-- A test file to demonstrate an incorrect recording of >> +-- getmetatable() for I/O handlers. >> +--https://github.com/LuaJIT/LuaJIT/issues/1279 > Nit: Usually the link is right before `tap.test()` declaration. Fixed. > >> + >> +jit.opt.start("hotloop=1") > Nit: Please use single quotes. Fixed. > >> + >> +local obj = io.stdout > Minor: I would rather name the variable like the corresponding type -- > `ud_io_file`. Feel free to ignore. Fixed. > >> +local getmetatable = getmetatable >> + >> +for _ = 1, 4 do >> + _ = getmetatable(obj) >> +end >> + >> +test:ok(true, 'getmetatable() recording is correct') > Let's check the resulted metatable too. (We can use `test:samevalues()` > here, for example.) Fixed. > >> + >> +test:done(true) >> -- >> 2.34.1 >> > [1]:https://github.com/LuaJIT/LuaJIT/issues/1279#issuecomment-2382392847 > --------------KYpCsdmOxbbIrerhuUW1kcVm Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Sergey,

please see below.

Updated version has been force-pushed.

Sergey

On 12.11.2024 22:23, Sergey Kaplun wrote:
Hi, Sergey!
Thanks for the patch!
Nice catch!

Please, consider my comments below.

On 07.11.24, Sergey Bronnikov wrote:

From: Sergey Bronnikov <sergeyb@tarantool.org>

The patch fixes a problem with recording `getmetatable()`
for I/O object: recording of `getmetatable` call with a file

Typo: s<I/O><an I/O>
Typo: s/recording of/recording the/
Typo: s/a file/file/
Fixed.


descriptors represented by userdata object `UDTYPE_IO_FILE`
(like `io.stdout`) leads to violation of assertion in
`rec_check_slots`.

Nit:
| ... by the userdata object `UDTYPE_IO_FILE` (like `io.stdout`) always
| stores `nil` instead of the given metatable. This leads to the
| violation of the assertion in `rec_check_slots`.


Note, the problem was fixed upstream in different manner, see
commit 5141cbc20c43

Nit: Please, use full commit hash.

Fixed, but usually ten characters is enough [1]:

> Generally, eight to ten characters are more than enough to be unique within a project. For example, as of February 2019, the Linux kernel (which is a fairly sizable project) has over 875,000 commits and almost seven million objects in its object database, with no two objects whose SHA-1s are identical in the first 12 characters.

1. https://git-scm.com/book/en/v2/Git-Tools-Revision-Selection

Also, please describe that specialized recording (introduced in the
upstream) lacks the check of the metatable precense. So, the fix in
upstream is incomplete (according to the comment [1]).

Added:


    Note, the problem was fixed upstream in different manner, see
    commit 5141cbc20c43921778ff36be541c15888bee8ee3
    ("Fix compiliation of getmetatable() for UDTYPE_IO_FILE.").
    Note, the specialization omits the check of `__metatable` field
    precense and the check for the metatable itself. So, if we change
    the metatable on the object after the trace is compiled, the trace
    becomes invalid. The proposed test demonstrates these cases as
    well.


Also, please add both testcases from the issue comment [1] (don't forget
to restore metatable after the first of them).
Added.

      

        
("Fix compiliation of getmetatable() for UDTYPE_IO_FILE.").
---
 src/lj_record.c                               |  2 +-
 ...-incorrect-recording-getmetatable.test.lua | 21 +++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua

diff --git a/src/lj_record.c b/src/lj_record.c
index cc97bdf9..7181b72a 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c


      

      
<snipped>



      

        
diff --git a/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua b/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua
new file mode 100644
index 00000000..8bf22ca7
--- /dev/null
+++ b/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua
@@ -0,0 +1,21 @@
+local tap = require('tap')
+
+local test = tap.test('lj-1279-incorrect-recording-getmetatable')
+test:plan(1)
+
+-- A test file to demonstrate an incorrect recording of
+-- getmetatable() for I/O handlers.
+-- https://github.com/LuaJIT/LuaJIT/issues/1279


      

      
Nit: Usually the link is right before `tap.test()` declaration.

    

    Fixed.

    

      



      

        
+
+jit.opt.start("hotloop=1")


      

      
Nit: Please use single quotes.

    

    Fixed.

    

      



      

        
+
+local obj = io.stdout


      

      
Minor: I would rather name the variable like the corresponding type --
`ud_io_file`. Feel free to ignore.

    

    
Fixed.

    


    

    

      



      

        
+local getmetatable = getmetatable
+
+for _ = 1, 4 do
+  _ = getmetatable(obj)
+end
+
+test:ok(true, 'getmetatable() recording is correct')


      

      
Let's check the resulted metatable too. (We can use `test:samevalues()`
here, for example.)

    

    Fixed.

    

      



      

        
+
+test:done(true)
-- 
2.34.1



      

      
[1]: https://github.com/LuaJIT/LuaJIT/issues/1279#issuecomment-2382392847



    

  
  


--------------KYpCsdmOxbbIrerhuUW1kcVm--