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 7026FE4EE28; Wed, 11 Dec 2024 16:58:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7026FE4EE28 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1733925511; bh=DRZfJeOhtPzED3GYIj9A3xvqJwzoRJzuhIkumTNAw3s=; 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=Ctq2T42AboDUtdj6T4MG1+vw9XukfSLVtIf9fsKdDFPZfUMwjFQx1OkmiPcnb3MqN VfkV9QBSuyaG/HZ623ZlBck4c1uYw9rc8beNdQ39EyFwxZsniFXe5Z1i8ViiOr2YaN tbM7P49xI2HGzcLM1yMqWhMqP2RDEYmj1Pg6XcOA= Received: from send241.i.mail.ru (send241.i.mail.ru [95.163.59.80]) (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 CA19E6E6F19 for ; Wed, 11 Dec 2024 16:58:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org CA19E6E6F19 Received: by exim-smtp-598667c6b5-9485p with esmtpa (envelope-from ) id 1tLNES-0000000022r-3Y8G; Wed, 11 Dec 2024 16:58:29 +0300 Content-Type: multipart/alternative; boundary="------------B8DbmjuiY9BHRNOHHrpq5JG0" Message-ID: Date: Wed, 11 Dec 2024 16:58:28 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Sergey Kaplun References: <76c9c163c365aec70741162ab83fdfa6385a9118.1730976041.git.sergeyb@tarantool.org> <19c0c442-68a9-4181-8424-7d3e202289f9@tarantool.org> Content-Language: en-US In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9B739379E53DE7095F44A8798952A5D7C8F5B4783BF7266F1182A05F538085040AC83C41B923BA50B3DE06ABAFEAF67059FF1BA7926C6F2AB9982D0576537AEA193B27CBAA3F87F55 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7C632C54049CA6CEBEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006376B6794A086D6ADA58638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D82ABD17C64160A4920ED690FC16A53D143D71882D3EF36C2ACC7F00164DA146DAFE8445B8C89999728AA50765F7900637C17F2030349B6B04389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC86089696B24BB1D19F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C4CB6874B0BCFF0B89735652A29929C6C4AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C3B88863A07377B48BBA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF17B107DEF921CE791DD303D21008E298D5E8D9A59859A8B6D082881546D9349175ECD9A6C639B01B78DA827A17800CE7F05A7CAACACB19EC731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5EAEF86309114BE6A5002B1117B3ED69689CD2474E4FEE993219207EC0A953D2C823CB91A9FED034534781492E4B8EEAD37F46C620FF2CAEEBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF377D0243849664B17EF68A7FC9B9F17142D50CE9B4E3DB9D8B235F7CFB4CF0CDACCD0F0CDF7B3897A0626FDB23E8EA706DCAD5F8F7DCCB6FC2465D9467A6E535890ED8EEFC9EF835C226CC413062362A913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojlzf/Y03FLlrC3WGPkqRTNw== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D61404ECB0C3D5E299088F378A8CA21F699D64C9A67880EF51F320152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 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: 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. --------------B8DbmjuiY9BHRNOHHrpq5JG0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Sergey, fixed and force-pushed. Sergey On 10.12.2024 12:32, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the fixes! > LGTM, after fixing a bunch of comments below. > >> 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..dc01307e >> --- /dev/null >> +++ b/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua >> @@ -0,0 +1,74 @@ >> +local tap = require('tap') >> +-- A test file to demonstrate an incorrect recording of >> +-- `getmetatable()` for I/O handlers. >> +--https://github.com/LuaJIT/LuaJIT/issues/1279 >> +local test = tap.test('lj-1279-incorrect-recording-getmetatable'):skipcond({ >> + ['Test requires JIT enabled'] = not jit.status(), >> +}) >> + >> +test:plan(6) >> + >> +jit.opt.start('hotloop=1') >> + >> +local ud_io_file = io.stdout >> +local getmetatable = getmetatable >> + >> +local function rec_getmetatable(obj) >> + local res >> + for _ = 1, 4 do >> + res = getmetatable(obj) >> + end >> + return res >> +end >> + >> +-- The testcase to demonstrate a problem by comparing metatable > Typo: s/metatable/the metatable/ --- a/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua +++ b/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua @@ -21,8 +21,9 @@ local function rec_getmetatable(obj)    return res  end --- The testcase to demonstrate a problem by comparing metatable --- returned by two versions of `getmetatable()`: compiled and not. +-- The testcase to demonstrate a problem by comparing the +-- metatable returned by two versions of `getmetatable()`: +-- compiled and not.  local mt_orig = debug.getmetatable(ud_io_file)  assert(type(mt_orig) == 'table') > >> +-- returned by two versions of `getmetatable()`: compiled and not. >> + >> +local mt_orig = debug.getmetatable(ud_io_file) >> +assert(type(mt_orig) == 'table') >> + >> +local mt_rec = {} >> +for i = 1, 4 do >> + mt_rec[i] = getmetatable(ud_io_file) >> +end >> +mt_rec[5] = mt_orig >> + >> +test:ok(true, 'getmetatable() recording is correct') >> +test:samevalues(mt_rec, 'metatables are the same') >> + >> +-- Restore metatable. >> +debug.setmetatable(ud_io_file, mt_orig) >> +assert(type(mt_orig) == 'table') > This restoring is excess since we don't change the metatable yet. Ok, updated: @@ -36,17 +37,13 @@ mt_rec[5] = mt_orig  test:ok(true, 'getmetatable() recording is correct')  test:samevalues(mt_rec, 'metatables are the same') --- Restore metatable. -debug.setmetatable(ud_io_file, mt_orig) -assert(type(mt_orig) == 'table') - > >> + >> +-- The testcase to demonstrate a problem by setting metatable for > Typo: s/metatable/the metatable/ Fixed: @@ -40,8 +41,8 @@ test:samevalues(mt_rec, 'metatables are the same')  debug.setmetatable(ud_io_file, mt_orig)  assert(type(mt_orig) == 'table') --- The testcase to demonstrate a problem by setting metatable for --- `io.stdout` to a string. +-- The testcase to demonstrate a problem by setting the metatable +-- for `io.stdout` to a string.  -- Compile `getmetatable()`, it is expected metatable has  -- a `table` type. > >> +-- `io.stdout` to a string. >> + >> +-- Compile `getmetatable()`, it is expected metatable has >> +-- a `table` type. >> +rec_getmetatable(ud_io_file) >> +-- Set a custom metatable to a string. > Minor: I would rephrase this like the following: > | Set IO metatable to a string. > "Custom" isn't clear for me. Fixed:  -- Compile `getmetatable()`, it is expected metatable has  -- a `table` type.  rec_getmetatable(ud_io_file) --- Set a custom metatable to a string. +-- Set IO metatable to a string.  local mt = 'IO metatable'  getmetatable(ud_io_file).__metatable = mt  test:is(getmetatable(ud_io_file), mt, 'getmetatable() is correct') >> +local mt = 'IO metatable' >> +getmetatable(ud_io_file).__metatable = mt >> +test:is(getmetatable(ud_io_file), mt, 'getmetatable() is correct') >> +test:is(rec_getmetatable(ud_io_file), mt, 'compiled getmetatable() is correct') >> + >> +-- Restore metatable. >> +debug.setmetatable(ud_io_file, mt_orig) >> +assert(type(mt_orig) == 'table') > It is better also to restore the JIT state here, ie. call `jit.flush()` > and reset hotcounters via `jit.opt.start('hotloop=1')` to be in sync > with the code (we want to compile a trace again). Added: @@ -55,9 +56,11 @@ test:is(rec_getmetatable(ud_io_file), mt, 'compiled getmetatable() is correct')  -- Restore metatable.  debug.setmetatable(ud_io_file, mt_orig)  assert(type(mt_orig) == 'table') +jit.flush() +jit.opt.start('hotloop=1') >> + >> +-- The testcase to demonstrate a problem by removing metatable for > Typo: s/metatable/the metatable/ Fixed: @@ -56,8 +57,8 @@ test:is(rec_getmetatable(ud_io_file), mt, 'compiled getmetatable() is correct')  debug.setmetatable(ud_io_file, mt_orig)  assert(type(mt_orig) == 'table') --- The testcase to demonstrate a problem by removing metatable for --- `io.stdout` and calling garbage collector. +-- The testcase to demonstrate a problem by removing the metatable +-- for `io.stdout` and calling garbage collector.  -- Compile `getmetatable()`, it is expected metatable has  -- a `table` type. >> +-- `io.stdout` and calling garbage collector. > Typo: s/garbage collector/the garbage collector/ Fixed: @@ -56,8 +57,8 @@ test:is(rec_getmetatable(ud_io_file), mt, 'compiled getmetatable() is correct')  debug.setmetatable(ud_io_file, mt_orig)  assert(type(mt_orig) == 'table') --- The testcase to demonstrate a problem by removing metatable for --- `io.stdout` and calling garbage collector. +-- The testcase to demonstrate a problem by removing the metatable +-- for `io.stdout` and calling the garbage collector.  -- Compile `getmetatable()`, it is expected metatable has  -- a `table` type. > >> + >> +-- Compile `getmetatable()`, it is expected metatable has >> +-- a `table` type. >> +rec_getmetatable(ud_io_file) >> +-- Delete metatable. >> +debug.setmetatable(ud_io_file, nil) >> +collectgarbage() >> +test:is(getmetatable(ud_io_file), nil, 'getmetatable() is correct') >> +test:is(rec_getmetatable(ud_io_file), nil, 'compiled getmetatable() is correct') >> + >> +-- Restore metatable. >> +debug.setmetatable(ud_io_file, mt_orig) >> + >> +test:done(true) > On 22.11.24, Sergey Bronnikov wrote: >> Hi, Sergey, >> >> please see below. >> >> Updated version has been force-pushed. >> >> Sergey >> > > >>> [1]:https://github.com/LuaJIT/LuaJIT/issues/1279#issuecomment-2382392847 >>> --------------B8DbmjuiY9BHRNOHHrpq5JG0 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Sergey,

fixed and force-pushed.

Sergey

On 10.12.2024 12:32, Sergey Kaplun wrote:
Hi, Sergey!
Thanks for the fixes!
LGTM, after fixing a bunch of comments below.


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..dc01307e
--- /dev/null
+++ b/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua
@@ -0,0 +1,74 @@
+local tap = require('tap')
+-- A test file to demonstrate an incorrect recording of
+-- `getmetatable()` for I/O handlers.
+-- https://github.com/LuaJIT/LuaJIT/issues/1279
+local test = tap.test('lj-1279-incorrect-recording-getmetatable'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(6)
+
+jit.opt.start('hotloop=1')
+
+local ud_io_file = io.stdout
+local getmetatable = getmetatable
+
+local function rec_getmetatable(obj)
+  local res
+  for _ = 1, 4 do
+    res = getmetatable(obj)
+  end
+  return res
+end
+
+-- The testcase to demonstrate a problem by comparing metatable

Typo: s/metatable/the metatable/
--- a/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua
+++ b/test/tarantool-tests/lj-1279-incorrect-recording-getmetatable.test.lua
@@ -21,8 +21,9 @@ local function rec_getmetatable(obj)
   return res
 end
 
--- The testcase to demonstrate a problem by comparing metatable
--- returned by two versions of `getmetatable()`: compiled and not.
+-- The testcase to demonstrate a problem by comparing the
+-- metatable returned by two versions of `getmetatable()`:
+-- compiled and not.
 
 local mt_orig = debug.getmetatable(ud_io_file)
 assert(type(mt_orig) == 'table')


+-- returned by two versions of `getmetatable()`: compiled and not.
+
+local mt_orig = debug.getmetatable(ud_io_file)
+assert(type(mt_orig) == 'table')
+
+local mt_rec = {}
+for i = 1, 4 do
+  mt_rec[i] = getmetatable(ud_io_file)
+end
+mt_rec[5] = mt_orig
+
+test:ok(true, 'getmetatable() recording is correct')
+test:samevalues(mt_rec, 'metatables are the same')
+
+-- Restore metatable.
+debug.setmetatable(ud_io_file, mt_orig)
+assert(type(mt_orig) == 'table')

This restoring is excess since we don't change the metatable yet.

Ok, updated:

@@ -36,17 +37,13 @@ mt_rec[5] = mt_orig
 test:ok(true, 'getmetatable() recording is correct')
 test:samevalues(mt_rec, 'metatables are the same')
 
--- Restore metatable.
-debug.setmetatable(ud_io_file, mt_orig)
-assert(type(mt_orig) == 'table')
-



+
+-- The testcase to demonstrate a problem by setting metatable for

Typo: s/metatable/the metatable/

Fixed:


@@ -40,8 +41,8 @@ test:samevalues(mt_rec, 'metatables are the same')
 debug.setmetatable(ud_io_file, mt_orig)
 assert(type(mt_orig) == 'table')
 
--- The testcase to demonstrate a problem by setting metatable for
--- `io.stdout` to a string.
+-- The testcase to demonstrate a problem by setting the metatable
+-- for `io.stdout` to a string.
 
 -- Compile `getmetatable()`, it is expected metatable has
 -- a `table` type.


+-- `io.stdout` to a string.
+
+-- Compile `getmetatable()`, it is expected metatable has
+-- a `table` type.
+rec_getmetatable(ud_io_file)
+-- Set a custom metatable to a string.

Minor: I would rephrase this like the following:
| Set IO metatable to a string.
"Custom" isn't clear for me.

Fixed:

 -- Compile `getmetatable()`, it is expected metatable has
 -- a `table` type.
 rec_getmetatable(ud_io_file)
--- Set a custom metatable to a string.
+-- Set IO metatable to a string.
 local mt = 'IO metatable'
 getmetatable(ud_io_file).__metatable = mt
 test:is(getmetatable(ud_io_file), mt, 'getmetatable() is correct')


      

        
+local mt = 'IO metatable'
+getmetatable(ud_io_file).__metatable = mt
+test:is(getmetatable(ud_io_file), mt, 'getmetatable() is correct')
+test:is(rec_getmetatable(ud_io_file), mt, 'compiled getmetatable() is correct')
+
+-- Restore metatable.
+debug.setmetatable(ud_io_file, mt_orig)
+assert(type(mt_orig) == 'table')


      

      
It is better also to restore the JIT state here, ie. call `jit.flush()`
and reset hotcounters via `jit.opt.start('hotloop=1')` to be in sync
with the code (we want to compile a trace again).


    

    
Added:

    


    

    
@@ -55,9 +56,11 @@ test:is(rec_getmetatable(ud_io_file), mt,
      'compiled getmetatable() is correct')

       -- Restore metatable.

       debug.setmetatable(ud_io_file, mt_orig)

       assert(type(mt_orig) == 'table')

      +jit.flush()

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

       

    

    

      

      

        
+
+-- The testcase to demonstrate a problem by removing metatable for


      

      
Typo: s/metatable/the metatable/


    

    
Fixed:

    


    

    @@ -56,8 +57,8 @@ test:is(rec_getmetatable(ud_io_file), mt,
    'compiled getmetatable() is correct')

     debug.setmetatable(ud_io_file, mt_orig)

     assert(type(mt_orig) == 'table')

     

    --- The testcase to demonstrate a problem by removing metatable for

    --- `io.stdout` and calling garbage collector.

    +-- The testcase to demonstrate a problem by removing the metatable

    +-- for `io.stdout` and calling garbage collector.

     

     -- Compile `getmetatable()`, it is expected metatable has

     -- a `table` type.

    

      

      

        
+-- `io.stdout` and calling garbage collector.


      

      
Typo: s/garbage collector/the garbage collector/

    

    
Fixed:

    


    

    @@ -56,8 +57,8 @@ test:is(rec_getmetatable(ud_io_file), mt,
    'compiled getmetatable() is correct')

     debug.setmetatable(ud_io_file, mt_orig)

     assert(type(mt_orig) == 'table')

     

    --- The testcase to demonstrate a problem by removing metatable for

    --- `io.stdout` and calling garbage collector.

    +-- The testcase to demonstrate a problem by removing the metatable

    +-- for `io.stdout` and calling the garbage collector.

     

     -- Compile `getmetatable()`, it is expected metatable has

     -- a `table` type.

    

      



      

        
+
+-- Compile `getmetatable()`, it is expected metatable has
+-- a `table` type.
+rec_getmetatable(ud_io_file)
+-- Delete metatable.
+debug.setmetatable(ud_io_file, nil)
+collectgarbage()
+test:is(getmetatable(ud_io_file), nil, 'getmetatable() is correct')
+test:is(rec_getmetatable(ud_io_file), nil, 'compiled getmetatable() is correct')
+
+-- Restore metatable.
+debug.setmetatable(ud_io_file, mt_orig)
+
+test:done(true)


      

      
On 22.11.24, Sergey Bronnikov wrote:


      

        
Hi, Sergey,

please see below.

Updated version has been force-pushed.

Sergey



      

      
<snipped>



      

        

          

            

          

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



        

      

      

    

  
  


--------------B8DbmjuiY9BHRNOHHrpq5JG0--