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

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

[Sergey Bronnikov via Tarantool-patches]

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

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

[Sergey Bronnikov via Tarantool-patches]

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

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

[Sergey Kaplun via 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