<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey,</p>
<p>please see below.</p>
<p>Updated version has been force-pushed.</p>
<p>Sergey<br>
</p>
<div class="moz-cite-prefix">On 12.11.2024 22:23, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite" cite="mid:ZzOrKAKgndu9eeFv@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the patch!
Nice catch!
Please, consider my comments below.
On 07.11.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">From: Sergey Bronnikov <a class="moz-txt-link-rfc2396E" href="mailto:sergeyb@tarantool.org"><sergeyb@tarantool.org></a>
The patch fixes a problem with recording `getmetatable()`
for I/O object: recording of `getmetatable` call with a file
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s<I/O><an I/O>
Typo: s/recording of/recording the/
Typo: s/a file/file/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZzOrKAKgndu9eeFv@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">descriptors represented by userdata object `UDTYPE_IO_FILE`
(like `io.stdout`) leads to violation of assertion in
`rec_check_slots`.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
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`.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Note, the problem was fixed upstream in different manner, see
commit 5141cbc20c43
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Please, use full commit hash.</pre>
</blockquote>
<p>Fixed, but usually ten characters is enough [1]:</p>
<p>> 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.</p>
<p>1. <a class="moz-txt-link-freetext" href="https://git-scm.com/book/en/v2/Git-Tools-Revision-Selection">https://git-scm.com/book/en/v2/Git-Tools-Revision-Selection</a></p>
<p><span style="white-space: pre-wrap">
</span></p>
<blockquote type="cite" cite="mid:ZzOrKAKgndu9eeFv@root">
<pre class="moz-quote-pre" wrap="">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]).</pre>
</blockquote>
<p>Added:</p>
<p><br>
</p>
<p> Note, the problem was fixed upstream in different manner, see<br>
commit 5141cbc20c43921778ff36be541c15888bee8ee3<br>
("Fix compiliation of getmetatable() for UDTYPE_IO_FILE.").<br>
Note, the specialization omits the check of `__metatable`
field<br>
precense and the check for the metatable itself. So, if we
change<br>
the metatable on the object after the trace is compiled, the
trace<br>
becomes invalid. The proposed test demonstrates these cases as<br>
well.<br>
</p>
<blockquote type="cite" cite="mid:ZzOrKAKgndu9eeFv@root">
<pre class="moz-quote-pre" wrap="">
Also, please add both testcases from the issue comment [1] (don't forget
to restore metatable after the first of them).
</pre>
</blockquote>
Added.<br>
<blockquote type="cite" cite="mid:ZzOrKAKgndu9eeFv@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">("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
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
<snipped>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.
+-- <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1279">https://github.com/LuaJIT/LuaJIT/issues/1279</a>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Usually the link is right before `tap.test()` declaration.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZzOrKAKgndu9eeFv@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+jit.opt.start("hotloop=1")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Please use single quotes.</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZzOrKAKgndu9eeFv@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+local obj = io.stdout
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Minor: I would rather name the variable like the corresponding type --
`ud_io_file`. Feel free to ignore.</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZzOrKAKgndu9eeFv@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+local getmetatable = getmetatable
+
+for _ = 1, 4 do
+ _ = getmetatable(obj)
+end
+
+test:ok(true, 'getmetatable() recording is correct')
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Let's check the resulted metatable too. (We can use `test:samevalues()`
here, for example.)</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZzOrKAKgndu9eeFv@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+
+test:done(true)
--
2.34.1
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
[1]: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1279#issuecomment-2382392847">https://github.com/LuaJIT/LuaJIT/issues/1279#issuecomment-2382392847</a>
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>