Re: [Tarantool-patches] [PATCH luajit] Fix memory probing allocator to check for valid end address, too.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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


Hi, Sergey!
Thanks for the comments!
 
> 
>>Hi, Maxim!
>>Thanks for the fixes!
>>LGTM, just a few typos.
>>
>>On 07.06.23, Maxim Kokryashkin wrote:
>>>
>>> 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
>>> >><test/tarantool-tests/gh-4199-gc64-fuse.test.lua> or
>>> >><test/tarantool-tests/gh-6098-fix-side-exit-patching-on-arm64.test.lua>
>>> >>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
>>
>>Typo: s/occurrs/occurs/
>Fixed, thanks! Branch is force-pushed, here is the diff:
>=============================================
>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
>index 9e31ed5e..a228651b 100644
>--- a/test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua
>+++ b/test/tarantool-tests/lj-445-fix-memory-probing-allocator.test.lua
>@@ -25,7 +25,7 @@ collectgarbage('stop')
> -- 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
>+-- When this issue occurs 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.
> --
>=============================================
>>
>>> >+-- failures in checks that rely on pointers being 32-bit.
>>
>>Typo: s/checks/the checks/
>That’s not a typo, ignoring. (Double-checked via Quillbot)
>>
>>> >+-- 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.
>>> >=============================================
>>
>><snipped>
>>
>>> >>
>>> >>--
>>> >>Best regards,
>>> >>Sergey Kaplun
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
>--
>Best regards,
>Maxim Kokryashkin

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