Re: [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 6610 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 10319 bytes --]