

<HTML><BODY><div>Hi!</div><div>Thanks for the review!</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16860597800584944851_BODY">Hi, Maxim!<br>Thanks for the patch!<br>The patch is LGTM except a few insiginificant nits below.<br><br>But I'm wondering: can we examine a test case mentioned in the [1]?<br>I.e. create a really long trace, near the upper bound of the 2GB, so<br>its results become meaningless? You may take a look into<br><test/tarantool-tests/gh-4199-gc64-fuse.test.lua> or<br><test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua><br>for the inspiration.<br><br>This is desired to show actual problem, and not changes in some<br>synthetic behaviour.</div></div></div></div></blockquote><div>As we discussed offline, I’ve added the following comment, branch is force-pushed:</div><div>=============================================</div><div><div><div>+-- XXX: This test allocates `cdata` objects, but in real world</div><div>+-- scenarios it can be any object that is allocated with</div><div>+-- LuaJIT's allocator, including, for example, trace, if it</div><div>+-- has been allocated close enough to the memory region</div><div>+-- upper bound and if it is long enough.</div><div>+--</div><div>+-- When this issue occurrs with a trace, it may lead to</div><div>+-- failures in checks that rely on pointers being 32-bit.</div><div>+-- For example, you can see one here: src/lj_asm_x86.h:370.</div><div>+--</div><div>+-- Although it is nice to have a reproducer that shows how</div><div>+-- that issue can affect a non-synthetic execution, it is really</div><div>+-- hard to achieve the described situation with traces because</div><div>+-- allocations are hint-based and there is no robust enough</div><div>+-- way to create a deterministic test for this behavior.</div><div>=============================================</div></div></div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>On 31.05.23, Maxim Kokryashkin wrote:<br>> From: Mike Pall <mike><br>><br>> (cherry-picked from commit 646148e747759f0af3b47f9bd287cedd7e174631)<br>><br>> Before the patch `mmap_probe` only checked if the allocated chunk<br>> start was within the 2^LJ_ALLOC_MBITS bytes region. However, if the<br>> chunk is big enough, its end can reach outside of that region. This<br>> patch adds the corresponding check, to avoid such situations.<br>><br>> Maxim Kokryashkin:<br>> * added the description and the test for the problem<br>><br>> Part of tarantool/tarantool#8516<br>> ---<br>> Branch: <a href="https://github.com/tarantool/luajit/tree/fckxorg/lj-445-fix-memory-probing-allocator" target="_blank">https://github.com/tarantool/luajit/tree/fckxorg/lj-445-fix-memory-probing-allocator</a><br>> PR: <a href="https://github.com/tarantool/tarantool/pull/8720" target="_blank">https://github.com/tarantool/tarantool/pull/8720</a><br>> LuaJIT issue: <a href="https://github.com/LuaJIT/LuaJIT/issues/445" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/445</a><br>><br>> src/lj_alloc.c | 3 +-<br>> ...-445-fix-memory-probing-allocator.test.lua | 32 +++++++++++++++++++<br>> 2 files changed, 34 insertions(+), 1 deletion(-)<br>> create mode 100644 test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua<br>><br>> diff --git a/src/lj_alloc.c b/src/lj_alloc.c<br>> index ffcd019b..f7039b5b 100644<br>> --- a/src/lj_alloc.c<br>> +++ b/src/lj_alloc.c<br><br><snipped><br><br>> diff --git a/test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua b/test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua<br>> new file mode 100644<br>> index 00000000..44763e38<br>> --- /dev/null<br>> +++ b/test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua<br>> @@ -0,0 +1,32 @@<br>> +local tap = require('tap')<br>> +local ffi = require('ffi')<br>> +local test = tap.test('lj-445-fix-memory-probing-allocator'):skipcond({<br>> + ['Unlikely to hit beyond the upper bound for GC64'] = ffi.abi('gc64'),<br>> +})<br>> +<br>> +local bit = require('bit')<br>> +local shr = bit.rshift<br>> +local uintptr_t = ffi.typeof('uintptr_t')<br>> +<br>> +-- Due to limitations in the x64 compiler backend, max memory limit is<br><br>Minor: comment line width is more than 66 symbols.</div></div></div></div></blockquote><div>Fixed.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +-- two times lower when JIT is not disabled entirely.<br>> +local HAS_JIT = jit.status()<br>> +local LJ_ALLOC_MBITS = HAS_JIT and 31 or 32<br>> +local MAX_GB = HAS_JIT and 2 or 4<br>> +<br>> +test:plan(MAX_GB)<br>> +<br>> +-- Chomp memory in currently allocated GC space.<br>> +collectgarbage('stop')<br>> +<br>> +-- Every allocation must either result in a chunk that fits into the<br><br>Ditto.</div></div></div></div></blockquote><div>Fixed.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>> +-- `MAX_GB`-sized region entirely or return an OOM error.<br>> +for _ = 1, MAX_GB do<br>> + local status, result = pcall(ffi.new, 'char[?]', 1024 * 1024 * 1024)<br>> + if status then<br>> + local upper_bound = ffi.cast(uintptr_t, result) + ffi.sizeof(result)<br>> + test:ok(shr(upper_bound, LJ_ALLOC_MBITS) == 0, 'non-extended address')<br>> + else<br>> + test:ok(result == 'not enough memory', 'OOM encountered')<br>> + end<br>> +end<br><br>Nit: Mising `os.exit()`.<br><br>> --<br>> 2.40.1<br>><br><br>[1]: <a href="https://github.com/LuaJIT/LuaJIT/issues/445" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/445</a><br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div><div> </div></div></blockquote></BODY></HTML>