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

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

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/

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

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

Typo: s/metatable/the metatable/

> +-- `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.

> +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).

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

Typo: s/metatable/the metatable/

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

Typo: s/garbage collector/the garbage collector/

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

-- 
Best regards,
Sergey Kaplun