Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE
@ 2024-11-07 10:51 Sergey Bronnikov via Tarantool-patches
  2024-11-07 14:37 ` Sergey Bronnikov via Tarantool-patches
  2024-11-12 19:23 ` Sergey Kaplun via Tarantool-patches
  0 siblings, 2 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-11-07 10:51 UTC (permalink / raw)
  To: tarantool-patches, Sergey Kaplun, Maxim Kokryashkin

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
descriptors represented by userdata object `UDTYPE_IO_FILE`
(like `io.stdout`) leads to violation of assertion in
`rec_check_slots`.

Note, the problem was fixed upstream in different manner, see
commit 5141cbc20c43
("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
@@ -990,7 +990,7 @@ int lj_record_mm_lookup(jit_State *J, RecordIndex *ix, MMS mm)
     int udtype = udataV(&ix->tabv)->udtype;
     mt = tabref(udataV(&ix->tabv)->metatable);
     /* The metatables of special userdata objects are treated as immutable. */
-    if (udtype != UDTYPE_USERDATA) {
+    if (udtype > UDTYPE_IO_FILE) {
       cTValue *mo;
       if (LJ_HASFFI && udtype == UDTYPE_FFI_CLIB) {
 	/* Specialize to the C library namespace object. */
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
+
+jit.opt.start("hotloop=1")
+
+local obj = io.stdout
+local getmetatable = getmetatable
+
+for _ = 1, 4 do
+  _ = getmetatable(obj)
+end
+
+test:ok(true, 'getmetatable() recording is correct')
+
+test:done(true)
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE
  2024-11-07 10:51 [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE Sergey Bronnikov via Tarantool-patches
@ 2024-11-07 14:37 ` Sergey Bronnikov via Tarantool-patches
  2024-11-12 19:23 ` Sergey Kaplun via Tarantool-patches
  1 sibling, 0 replies; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-11-07 14:37 UTC (permalink / raw)
  To: Sergey Bronnikov, tarantool-patches, Sergey Kaplun, Maxim Kokryashkin

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

Branch: 
https://github.com/tarantool/luajit/tree/ligurio/gh-1279-recording-getmetatable

LJ issue: https://github.com/LuaJIT/LuaJIT/issues/1279


On 07.11.2024 13:51, 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
> descriptors represented by userdata object `UDTYPE_IO_FILE`
> (like `io.stdout`) leads to violation of assertion in
> `rec_check_slots`.
>
> Note, the problem was fixed upstream in different manner, see
> commit 5141cbc20c43
> ("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
> @@ -990,7 +990,7 @@ int lj_record_mm_lookup(jit_State *J, RecordIndex *ix, MMS mm)
>       int udtype = udataV(&ix->tabv)->udtype;
>       mt = tabref(udataV(&ix->tabv)->metatable);
>       /* The metatables of special userdata objects are treated as immutable. */
> -    if (udtype != UDTYPE_USERDATA) {
> +    if (udtype > UDTYPE_IO_FILE) {
>         cTValue *mo;
>         if (LJ_HASFFI && udtype == UDTYPE_FFI_CLIB) {
>   	/* Specialize to the C library namespace object. */
> 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
> +
> +jit.opt.start("hotloop=1")
> +
> +local obj = io.stdout
> +local getmetatable = getmetatable
> +
> +for _ = 1, 4 do
> +  _ = getmetatable(obj)
> +end
> +
> +test:ok(true, 'getmetatable() recording is correct')
> +
> +test:done(true)

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE
  2024-11-07 10:51 [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE Sergey Bronnikov via Tarantool-patches
  2024-11-07 14:37 ` Sergey Bronnikov via Tarantool-patches
@ 2024-11-12 19:23 ` Sergey Kaplun via Tarantool-patches
  2024-11-22 12:53   ` Sergey Bronnikov via Tarantool-patches
  1 sibling, 1 reply; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-11-12 19:23 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: tarantool-patches

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/

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

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

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

> ("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.

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

Nit: Please use single quotes.

> +
> +local obj = io.stdout

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

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

> +
> +test:done(true)
> -- 
> 2.34.1
> 

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

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE
  2024-11-12 19:23 ` Sergey Kaplun via Tarantool-patches
@ 2024-11-22 12:53   ` Sergey Bronnikov via Tarantool-patches
  2024-12-10  9:32     ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-11-22 12:53 UTC (permalink / raw)
  To: Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches

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

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
>

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE
  2024-11-22 12:53   ` Sergey Bronnikov via Tarantool-patches
@ 2024-12-10  9:32     ` Sergey Kaplun via Tarantool-patches
  2024-12-11 13:58       ` Sergey Bronnikov via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-12-10  9:32 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: Sergey Bronnikov, tarantool-patches

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE
  2024-12-10  9:32     ` Sergey Kaplun via Tarantool-patches
@ 2024-12-11 13:58       ` Sergey Bronnikov via Tarantool-patches
  2024-12-11 15:54         ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-12-11 13:58 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: Sergey Bronnikov, tarantool-patches

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE
  2024-12-11 13:58       ` Sergey Bronnikov via Tarantool-patches
@ 2024-12-11 15:54         ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-12-11 15:54 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: tarantool-patches, Sergey Bronnikov

Hi, Sergey!
Thanks for the patch and fixes!
LGTM

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-12-11 15:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-07 10:51 [Tarantool-patches] [PATCH luajit] Fix compilation of getmetatable() for UDTYPE_IO_FILE Sergey Bronnikov via Tarantool-patches
2024-11-07 14:37 ` Sergey Bronnikov via Tarantool-patches
2024-11-12 19:23 ` Sergey Kaplun via Tarantool-patches
2024-11-22 12:53   ` Sergey Bronnikov via Tarantool-patches
2024-12-10  9:32     ` Sergey Kaplun via Tarantool-patches
2024-12-11 13:58       ` Sergey Bronnikov via Tarantool-patches
2024-12-11 15:54         ` Maxim Kokryashkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox