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