[Tarantool-patches] [PATCH luajit v1 0/5] Handle all errors during stitching

[Tarantool-patches] [PATCH luajit v1 0/5] Handle all errors during stitching

[Sergey Kaplun via Tarantool-patches]

This patchset handles all possible error types that may be observed
during stitching. The first 2 patches add an option to enable table bump
optimization for CMake options and add it to the CI exotic build
correspondingly. The third patch enhances the <jit/parse.lua> testing
utility to return IRs of aborted trace as well. Also, the reason for the
abort of a trace may be examined. The last 2 patches handle all possible
errors during stitching and ensure that the output of `jit.dump()` is
valid for all cases.

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1166-errors-stitching
Related Issues:
* https://github.com/LuaJIT/LuaJIT/issues/1166
* https://github.com/LuaJIT/LuaJIT/pull/720
* https://github.com/LuaJIT/LuaJIT/issues/606
* https://github.com/tarantool/tarantool/issues/9924

Mike Pall (2):
  Handle all types of errors during trace stitching.
  Use generic trace error for OOM during trace stitching.

Sergey Kaplun (3):
  build: introduce option LUAJIT_ENABLE_TABLE_BUMP
  ci: add tablebump flavor for exotic builds
  test: allow `jit.parse` to return aborted traces

 .github/workflows/exotic-builds-testing.yml   | 10 ++-
 CMakeLists.txt                                | 17 ++++
 src/lj_ffrecord.c                             | 23 +++--
 test/tarantool-tests/CMakeLists.txt           |  1 +
 .../lj-1166-error-stitch-oom-ir-buff.test.lua | 83 ++++++++++++++++++
 ...j-1166-error-stitch-oom-snap-buff.test.lua | 85 +++++++++++++++++++
 .../lj-1166-error-stitch-table-bump.test.lua  | 38 +++++++++
 .../lj-1166-error-stitch/CMakeLists.txt       |  1 +
 .../lj-1166-error-stitch/mockalloc.c          | 51 +++++++++++
 .../lj-720-errors-before-stitch.test.lua      | 40 ++++++++-
 .../unit-jit-parse-abort.test.lua             | 38 +++++++++
 test/tarantool-tests/utils/jit/parse.lua      | 22 +++--
 12 files changed, 396 insertions(+), 13 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
 create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
 create mode 100644 test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
 create mode 100644 test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
 create mode 100644 test/tarantool-tests/unit-jit-parse-abort.test.lua

-- 
2.44.0

[Tarantool-patches] [PATCH luajit v1 1/5] build: introduce option LUAJIT_ENABLE_TABLE_BUMP

[Sergey Kaplun via Tarantool-patches]

This option enables table bump optimization if sink optimization is
enabled. The table bump optimization patches the bytecodes with a table
allocation on the trace recording if the recorded trace exceeds the size
of the allocated table. This optimization still has some bugs, so it is
disabled by default. For more details, see the comment in
<CMakeLists.txt>.

Needed for tarantool/tarantool#9924
---
 CMakeLists.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2355ce17..52014296 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -307,6 +307,23 @@ if(LUAJIT_ENABLE_COVERAGE)
   include(CodeCoverage)
 endif()
 
+# Enable table bump optimization. This optimization patches the
+# bytecodes with a table allocation on the trace recording if the
+# recorded trace exceeds the size of the allocated table.
+# This optimization still has some bugs, so it is disabled by
+# default. See also: https://github.com/LuaJIT/LuaJIT/issues/606.
+option(LUAJIT_ENABLE_TABLE_BUMP "Enable table bump optimization" OFF)
+if(LUAJIT_ENABLE_TABLE_BUMP)
+  # Within table bump optimization enabled (and due to our
+  # modification related to metrics), some offsets in `GG_State`
+  # stop fit in 12bit immediate. Hence, the build failed due to
+  # the DASM error (DASM_S_RANGE_I).
+  if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
+    message(FATAL_ERROR "Table Bump optimization is unsupported for aarch64")
+  endif()
+  AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_TABLE_BUMP)
+endif()
+
 # --- Main source tree ---------------------------------------------------------
 
 add_subdirectory(src)
-- 
2.44.0

Re: [Tarantool-patches] [PATCH luajit v1 1/5] build: introduce option LUAJIT_ENABLE_TABLE_BUMP

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, except for a few nits below.
On Mon, Apr 22, 2024 at 11:49:54AM UTC, Sergey Kaplun wrote:
> This option enables table bump optimization if sink optimization is
> enabled. The table bump optimization patches the bytecodes with a table
> allocation on the trace recording if the recorded trace exceeds the size
> of the allocated table. This optimization still has some bugs, so it is
> disabled by default. For more details, see the comment in
> <CMakeLists.txt>.
>
> Needed for tarantool/tarantool#9924
> ---
>  CMakeLists.txt | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 2355ce17..52014296 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -307,6 +307,23 @@ if(LUAJIT_ENABLE_COVERAGE)
>    include(CodeCoverage)
>  endif()
>
> +# Enable table bump optimization. This optimization patches the
> +# bytecodes with a table allocation on the trace recording if the
Typo: s/bytecodes/bytecode/
> +# recorded trace exceeds the size of the allocated table.
> +# This optimization still has some bugs, so it is disabled by
> +# default. See also: https://github.com/LuaJIT/LuaJIT/issues/606.
> +option(LUAJIT_ENABLE_TABLE_BUMP "Enable table bump optimization" OFF)
> +if(LUAJIT_ENABLE_TABLE_BUMP)
> +  # Within table bump optimization enabled (and due to our
Typo: s/Within/With/
> +  # modification related to metrics), some offsets in `GG_State`
> +  # stop fit in 12bit immediate. Hence, the build failed due to
> +  # the DASM error (DASM_S_RANGE_I).
> +  if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
> +    message(FATAL_ERROR "Table Bump optimization is unsupported for aarch64")
Typo: s/Bump/bump/
> +  endif()
> +  AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_TABLE_BUMP)
> +endif()
> +
>  # --- Main source tree ---------------------------------------------------------
>
>  add_subdirectory(src)
> --
> 2.44.0
>

Re: [Tarantool-patches] [PATCH luajit v1 1/5] build: introduce option LUAJIT_ENABLE_TABLE_BUMP

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Fixed your comments. See the iterative patch below.

On 05.05.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits below.
> On Mon, Apr 22, 2024 at 11:49:54AM UTC, Sergey Kaplun wrote:
> > This option enables table bump optimization if sink optimization is
> > enabled. The table bump optimization patches the bytecodes with a table
> > allocation on the trace recording if the recorded trace exceeds the size
> > of the allocated table. This optimization still has some bugs, so it is
> > disabled by default. For more details, see the comment in
> > <CMakeLists.txt>.
> >
> > Needed for tarantool/tarantool#9924
> > ---
> >  CMakeLists.txt | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index 2355ce17..52014296 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -307,6 +307,23 @@ if(LUAJIT_ENABLE_COVERAGE)
> >    include(CodeCoverage)
> >  endif()
> >
> > +# Enable table bump optimization. This optimization patches the
> > +# bytecodes with a table allocation on the trace recording if the
> Typo: s/bytecodes/bytecode/

Fixed.

> > +# recorded trace exceeds the size of the allocated table.
> > +# This optimization still has some bugs, so it is disabled by
> > +# default. See also: https://github.com/LuaJIT/LuaJIT/issues/606.
> > +option(LUAJIT_ENABLE_TABLE_BUMP "Enable table bump optimization" OFF)
> > +if(LUAJIT_ENABLE_TABLE_BUMP)
> > +  # Within table bump optimization enabled (and due to our
> Typo: s/Within/With/

Fixed.

> > +  # modification related to metrics), some offsets in `GG_State`
> > +  # stop fit in 12bit immediate. Hence, the build failed due to
> > +  # the DASM error (DASM_S_RANGE_I).
> > +  if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
> > +    message(FATAL_ERROR "Table Bump optimization is unsupported for aarch64")
> Typo: s/Bump/bump/

Fixed.

===================================================================
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 52014296..ce8d0311 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -308,18 +308,18 @@ if(LUAJIT_ENABLE_COVERAGE)
 endif()
 
 # Enable table bump optimization. This optimization patches the
-# bytecodes with a table allocation on the trace recording if the
+# bytecode with a table allocation on the trace recording if the
 # recorded trace exceeds the size of the allocated table.
 # This optimization still has some bugs, so it is disabled by
 # default. See also: https://github.com/LuaJIT/LuaJIT/issues/606.
 option(LUAJIT_ENABLE_TABLE_BUMP "Enable table bump optimization" OFF)
 if(LUAJIT_ENABLE_TABLE_BUMP)
-  # Within table bump optimization enabled (and due to our
+  # With table bump optimization enabled (and due to our
   # modification related to metrics), some offsets in `GG_State`
-  # stop fit in 12bit immediate. Hence, the build failed due to
-  # the DASM error (DASM_S_RANGE_I).
+  # stop fitting in 12bit immediate. Hence, the build failed due
+  # to the DASM error (`DASM_S_RANGE_I`).
   if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
-    message(FATAL_ERROR "Table Bump optimization is unsupported for aarch64")
+    message(FATAL_ERROR "Table bump optimization is unsupported for aarch64")
   endif()
   AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_TABLE_BUMP)
 endif()
===================================================================

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v1 1/5] build: introduce option LUAJIT_ENABLE_TABLE_BUMP

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey


thanks for the patch! Please see a question below.

On 22.04.2024 11:49, Sergey Kaplun wrote:
> This option enables table bump optimization if sink optimization is
> enabled. The table bump optimization patches the bytecodes with a table
> allocation on the trace recording if the recorded trace exceeds the size
> of the allocated table. This optimization still has some bugs, so it is
> disabled by default. For more details, see the comment in
> <CMakeLists.txt>.
>
> Needed for tarantool/tarantool#9924
> ---
>   CMakeLists.txt | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 2355ce17..52014296 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -307,6 +307,23 @@ if(LUAJIT_ENABLE_COVERAGE)
>     include(CodeCoverage)
>   endif()
>   
> +# Enable table bump optimization. This optimization patches the
> +# bytecodes with a table allocation on the trace recording if the
> +# recorded trace exceeds the size of the allocated table.
> +# This optimization still has some bugs, so it is disabled by
> +# default. See also:https://github.com/LuaJIT/LuaJIT/issues/606.
> +option(LUAJIT_ENABLE_TABLE_BUMP "Enable table bump optimization" OFF)
> +if(LUAJIT_ENABLE_TABLE_BUMP)
> +  # Within table bump optimization enabled (and due to our
> +  # modification related to metrics), some offsets in `GG_State`
> +  # stop fit in 12bit immediate. Hence, the build failed due to
> +  # the DASM error (DASM_S_RANGE_I).
> +  if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
> +    message(FATAL_ERROR "Table Bump optimization is unsupported for aarch64")

Table optimization works on all architectures supported by LuaJIT except 
aarch64, right?


> +  endif()
> +  AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_TABLE_BUMP)
> +endif()
> +
>   # --- Main source tree ---------------------------------------------------------
>   
>   add_subdirectory(src)

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

Re: [Tarantool-patches] [PATCH luajit v1 1/5] build: introduce option LUAJIT_ENABLE_TABLE_BUMP

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On 06.06.24, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> 
> thanks for the patch! Please see a question below.
> 
> On 22.04.2024 11:49, Sergey Kaplun wrote:
> > This option enables table bump optimization if sink optimization is
> > enabled. The table bump optimization patches the bytecodes with a table
> > allocation on the trace recording if the recorded trace exceeds the size
> > of the allocated table. This optimization still has some bugs, so it is
> > disabled by default. For more details, see the comment in
> > <CMakeLists.txt>.
> >
> > Needed for tarantool/tarantool#9924
> > ---
> >   CMakeLists.txt | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> >
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index 2355ce17..52014296 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -307,6 +307,23 @@ if(LUAJIT_ENABLE_COVERAGE)
> >     include(CodeCoverage)
> >   endif()
> >   
> > +# Enable table bump optimization. This optimization patches the
> > +# bytecodes with a table allocation on the trace recording if the
> > +# recorded trace exceeds the size of the allocated table.
> > +# This optimization still has some bugs, so it is disabled by
> > +# default. See also:https://github.com/LuaJIT/LuaJIT/issues/606.
> > +option(LUAJIT_ENABLE_TABLE_BUMP "Enable table bump optimization" OFF)
> > +if(LUAJIT_ENABLE_TABLE_BUMP)
> > +  # Within table bump optimization enabled (and due to our
> > +  # modification related to metrics), some offsets in `GG_State`
> > +  # stop fit in 12bit immediate. Hence, the build failed due to
> > +  # the DASM error (DASM_S_RANGE_I).
> > +  if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
> > +    message(FATAL_ERROR "Table Bump optimization is unsupported for aarch64")
> 
> Table optimization works on all architectures supported by LuaJIT except 
> aarch64, right?

In the upstream, it works on all architectures. We have a problem
building LuaJIT for aarch64 due to our metrics that too grows the
`GG_State`.

> 
> 
> > +  endif()
> > +  AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_TABLE_BUMP)
> > +endif()
> > +
> >   # --- Main source tree ---------------------------------------------------------
> >   
> >   add_subdirectory(src)

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v1 1/5] build: introduce option LUAJIT_ENABLE_TABLE_BUMP

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey


thanks for the answer. LGTM

On 13.06.2024 12:51, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
>
> On 06.06.24, Sergey Bronnikov wrote:
>> Hi, Sergey
>>
>>
>> thanks for the patch! Please see a question below.
>>
>> On 22.04.2024 11:49, Sergey Kaplun wrote:
>>> This option enables table bump optimization if sink optimization is
>>> enabled. The table bump optimization patches the bytecodes with a table
>>> allocation on the trace recording if the recorded trace exceeds the size
>>> of the allocated table. This optimization still has some bugs, so it is
>>> disabled by default. For more details, see the comment in
>>> <CMakeLists.txt>.
>>>
>>> Needed for tarantool/tarantool#9924
>>> ---
>>>    CMakeLists.txt | 17 +++++++++++++++++
>>>    1 file changed, 17 insertions(+)
>>>
>>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>>> index 2355ce17..52014296 100644
>>> --- a/CMakeLists.txt
>>> +++ b/CMakeLists.txt
>>> @@ -307,6 +307,23 @@ if(LUAJIT_ENABLE_COVERAGE)
>>>      include(CodeCoverage)
>>>    endif()
>>>    
>>> +# Enable table bump optimization. This optimization patches the
>>> +# bytecodes with a table allocation on the trace recording if the
>>> +# recorded trace exceeds the size of the allocated table.
>>> +# This optimization still has some bugs, so it is disabled by
>>> +# default. See also:https://github.com/LuaJIT/LuaJIT/issues/606.
>>> +option(LUAJIT_ENABLE_TABLE_BUMP "Enable table bump optimization" OFF)
>>> +if(LUAJIT_ENABLE_TABLE_BUMP)
>>> +  # Within table bump optimization enabled (and due to our
>>> +  # modification related to metrics), some offsets in `GG_State`
>>> +  # stop fit in 12bit immediate. Hence, the build failed due to
>>> +  # the DASM error (DASM_S_RANGE_I).
>>> +  if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
>>> +    message(FATAL_ERROR "Table Bump optimization is unsupported for aarch64")
>> Table optimization works on all architectures supported by LuaJIT except
>> aarch64, right?
> In the upstream, it works on all architectures. We have a problem
> building LuaJIT for aarch64 due to our metrics that too grows the
> `GG_State`.
>
>>
>>> +  endif()
>>> +  AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_TABLE_BUMP)
>>> +endif()
>>> +
>>>    # --- Main source tree ---------------------------------------------------------
>>>    
>>>    add_subdirectory(src)

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

[Tarantool-patches] [PATCH luajit v1 2/5] ci: add tablebump flavor for exotic builds

[Sergey Kaplun via Tarantool-patches]

This flavor enables the LUAJIT_ENABLE_TABLE_BUMP option to test table
bump optimization.

Needed for tarantool/tarantool#9924
---
 .github/workflows/exotic-builds-testing.yml | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
index 859603bd..6ce100f4 100644
--- a/.github/workflows/exotic-builds-testing.yml
+++ b/.github/workflows/exotic-builds-testing.yml
@@ -34,7 +34,7 @@ jobs:
         BUILDTYPE: [Debug, Release]
         ARCH: [ARM64, x86_64]
         GC64: [ON, OFF]
-        FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
+        FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind, tablebump]
         include:
           - BUILDTYPE: Debug
             CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
@@ -50,12 +50,20 @@ jobs:
             FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
           - FLAVOR: nounwind
             FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
+          - FLAVOR: tablebump
+            FLAVORFLAGS: -DLUAJIT_ENABLE_TABLE_BUMP=ON
         exclude:
           - ARCH: ARM64
             GC64: OFF
           # DUALNUM is default for ARM64, no need for additional testing.
           - FLAVOR: dualnum
             ARCH: ARM64
+          # Within table bump optimization enabled (and due to our modification
+          # related to metrics), some offsets in GG_State stop fit in 12bit
+          # immediate. Hence, the build failed due to the DASM error
+          # (DASM_S_RANGE_I).
+          - FLAVOR: tablebump
+            ARCH: ARM64
     runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
     name: >
       LuaJIT ${{ matrix.FLAVOR }}
-- 
2.44.0

Re: [Tarantool-patches] [PATCH luajit v1 2/5] ci: add tablebump flavor for exotic builds

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, except for the single nit below.
On Mon, Apr 22, 2024 at 11:49:55AM UTC, Sergey Kaplun wrote:
> This flavor enables the LUAJIT_ENABLE_TABLE_BUMP option to test table
> bump optimization.
>
> Needed for tarantool/tarantool#9924
> ---
>  .github/workflows/exotic-builds-testing.yml | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
> index 859603bd..6ce100f4 100644
> --- a/.github/workflows/exotic-builds-testing.yml
> +++ b/.github/workflows/exotic-builds-testing.yml
> @@ -34,7 +34,7 @@ jobs:
>          BUILDTYPE: [Debug, Release]
>          ARCH: [ARM64, x86_64]
>          GC64: [ON, OFF]
> -        FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
> +        FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind, tablebump]
>          include:
>            - BUILDTYPE: Debug
>              CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> @@ -50,12 +50,20 @@ jobs:
>              FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
>            - FLAVOR: nounwind
>              FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
> +          - FLAVOR: tablebump
> +            FLAVORFLAGS: -DLUAJIT_ENABLE_TABLE_BUMP=ON
>          exclude:
>            - ARCH: ARM64
>              GC64: OFF
>            # DUALNUM is default for ARM64, no need for additional testing.
>            - FLAVOR: dualnum
>              ARCH: ARM64
> +          # Within table bump optimization enabled (and due to our modification
Typo: s/Within/With/
> +          # related to metrics), some offsets in GG_State stop fit in 12bit
> +          # immediate. Hence, the build failed due to the DASM error
> +          # (DASM_S_RANGE_I).
> +          - FLAVOR: tablebump
> +            ARCH: ARM64
>      runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
>      name: >
>        LuaJIT ${{ matrix.FLAVOR }}
> --
> 2.44.0
>

Re: [Tarantool-patches] [PATCH luajit v1 2/5] ci: add tablebump flavor for exotic builds

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Fixed your comment. See the iterative patch below.

On 05.05.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for the single nit below.
> On Mon, Apr 22, 2024 at 11:49:55AM UTC, Sergey Kaplun wrote:
> > This flavor enables the LUAJIT_ENABLE_TABLE_BUMP option to test table
> > bump optimization.
> >
> > Needed for tarantool/tarantool#9924
> > ---

<snipped>

> >            # DUALNUM is default for ARM64, no need for additional testing.
> >            - FLAVOR: dualnum
> >              ARCH: ARM64
> > +          # Within table bump optimization enabled (and due to our modification
> Typo: s/Within/With/

Fixed.

> > +          # related to metrics), some offsets in GG_State stop fit in 12bit
> > +          # immediate. Hence, the build failed due to the DASM error
> > +          # (DASM_S_RANGE_I).
> > +          - FLAVOR: tablebump
> > +            ARCH: ARM64

===================================================================
diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
index 6ce100f4..70096439 100644
--- a/.github/workflows/exotic-builds-testing.yml
+++ b/.github/workflows/exotic-builds-testing.yml
@@ -58,10 +58,10 @@ jobs:
           # DUALNUM is default for ARM64, no need for additional testing.
           - FLAVOR: dualnum
             ARCH: ARM64
-          # Within table bump optimization enabled (and due to our modification
-          # related to metrics), some offsets in GG_State stop fit in 12bit
+          # With table bump optimization enabled (and due to our modification
+          # related to metrics), some offsets in GG_State stop fitting in 12bit
           # immediate. Hence, the build failed due to the DASM error
-          # (DASM_S_RANGE_I).
+          # (`DASM_S_RANGE_I`).
           - FLAVOR: tablebump
             ARCH: ARM64
     runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
===================================================================

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v1 2/5] ci: add tablebump flavor for exotic builds

[Sergey Bronnikov via Tarantool-patches]

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

Sergey,

thanks for the patch! LGTM

On 22.04.2024 11:49, Sergey Kaplun wrote:
> This flavor enables the LUAJIT_ENABLE_TABLE_BUMP option to test table
> bump optimization.
>
> Needed for tarantool/tarantool#9924
> ---
>   .github/workflows/exotic-builds-testing.yml | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
> index 859603bd..6ce100f4 100644
> --- a/.github/workflows/exotic-builds-testing.yml
> +++ b/.github/workflows/exotic-builds-testing.yml
> @@ -34,7 +34,7 @@ jobs:
>           BUILDTYPE: [Debug, Release]
>           ARCH: [ARM64, x86_64]
>           GC64: [ON, OFF]
> -        FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind]
> +        FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind, tablebump]
>           include:
>             - BUILDTYPE: Debug
>               CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> @@ -50,12 +50,20 @@ jobs:
>               FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON
>             - FLAVOR: nounwind
>               FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON
> +          - FLAVOR: tablebump
> +            FLAVORFLAGS: -DLUAJIT_ENABLE_TABLE_BUMP=ON
>           exclude:
>             - ARCH: ARM64
>               GC64: OFF
>             # DUALNUM is default for ARM64, no need for additional testing.
>             - FLAVOR: dualnum
>               ARCH: ARM64
> +          # Within table bump optimization enabled (and due to our modification
> +          # related to metrics), some offsets in GG_State stop fit in 12bit
> +          # immediate. Hence, the build failed due to the DASM error
> +          # (DASM_S_RANGE_I).
> +          - FLAVOR: tablebump
> +            ARCH: ARM64
>       runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}']
>       name: >
>         LuaJIT ${{ matrix.FLAVOR }}

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

[Tarantool-patches] [PATCH luajit v1 3/5] test: allow `jit.parse` to return aborted traces

[Sergey Kaplun via Tarantool-patches]

Now information about the abort of the trace is saved in the
`abort_reason` field of the corresponding structure. The
`jit.parse.finish()` returns now the second table containing aborted
traces. Each table key is a trace number containing an array of
potentially traces with this number, which was aborted.

Needed for tarantool/tarantool#9924
---
 .../unit-jit-parse-abort.test.lua             | 38 +++++++++++++++++++
 test/tarantool-tests/utils/jit/parse.lua      | 22 ++++++++---
 2 files changed, 55 insertions(+), 5 deletions(-)
 create mode 100644 test/tarantool-tests/unit-jit-parse-abort.test.lua

diff --git a/test/tarantool-tests/unit-jit-parse-abort.test.lua b/test/tarantool-tests/unit-jit-parse-abort.test.lua
new file mode 100644
index 00000000..91af5a56
--- /dev/null
+++ b/test/tarantool-tests/unit-jit-parse-abort.test.lua
@@ -0,0 +1,38 @@
+local tap = require('tap')
+local test = tap.test('unit-jit-parse'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
+})
+
+local jparse = require('utils').jit.parse
+
+-- XXX: Avoid other traces compilation due to hotcount collisions
+-- for predictable results.
+jit.off()
+jit.flush()
+
+test:plan(1)
+
+jit.on()
+-- We only need the abort reason in the test.
+jparse.start('t')
+
+-- XXX: A trace always has at least 3 IR constants: for `nil`,
+-- `false`, and `true`. Always fails to record with the set
+-- `maxirconst` limit.
+jit.opt.start('hotloop=1', 'maxirconst=1')
+
+for _ = 1, 3 do end
+
+local _, aborted_traces = jparse.finish()
+
+jit.off()
+
+assert(aborted_traces and aborted_traces[1], 'aborted trace is persisted')
+
+-- We tried to compile only one trace.
+local reason = aborted_traces[1][1].abort_reason
+
+test:like(reason, 'trace too long', 'abort reason is correct')
+
+test:done(true)
diff --git a/test/tarantool-tests/utils/jit/parse.lua b/test/tarantool-tests/utils/jit/parse.lua
index bcef5b35..0ce7f7c8 100644
--- a/test/tarantool-tests/utils/jit/parse.lua
+++ b/test/tarantool-tests/utils/jit/parse.lua
@@ -22,6 +22,7 @@ local function trace_new(n)
     parent = nil,
     parent_exitno = nil,
     is_stitched = false,
+    abort_reason = nil,
     start_loc = nil,
     bc = {},
     ir = {},
@@ -87,9 +88,17 @@ local header_handlers = {
     ctx.parsing_trace = nil
     ctx.parsing = nil
   end,
-  abort = function(ctx, trace_num)
+  abort = function(ctx, trace_num, line)
     local traces = ctx.traces
     assert(ctx.parsing_trace == trace_num)
+
+    local aborted_traces = ctx.aborted_traces
+    if not aborted_traces[trace_num] then
+      aborted_traces[trace_num] = {}
+    end
+    traces[trace_num].abort_reason = line:match('-- (.+)$')
+    table.insert(aborted_traces[trace_num], traces[trace_num])
+
     ctx.parsing_trace = nil
     ctx.parsing = nil
     traces[trace_num] = nil
@@ -137,11 +146,14 @@ end
 local JDUMP_FILE
 
 local function parse_jit_dump()
-  local ctx = {traces = {}}
+  local ctx = {
+    aborted_traces = {},
+    traces = {},
+  }
   for line in io.lines(JDUMP_FILE) do
     parse_line(ctx, line)
   end
-  return ctx.traces
+  return ctx.traces, ctx.aborted_traces
 end
 
 -- Start `jit.dump()` utility with the given flags, saving the
@@ -167,10 +179,10 @@ M.finish = function()
   -- Enable traces compilation for `jit.dump` back.
   jit.on(jdump.on, true)
   jit.on(jdump.off, true)
-  local traces = parse_jit_dump()
+  local traces, aborted_traces = parse_jit_dump()
   os.remove(JDUMP_FILE)
   JDUMP_FILE = nil
-  return traces
+  return traces, aborted_traces
 end
 
 -- Turn off compilation for the module to avoid side effects.
-- 
2.44.0

Re: [Tarantool-patches] [PATCH luajit v1 3/5] test: allow `jit.parse` to return aborted traces

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, except for a single nit below.
On Mon, Apr 22, 2024 at 11:49:56AM UTC, Sergey Kaplun wrote:
> Now information about the abort of the trace is saved in the
> `abort_reason` field of the corresponding structure. The
> `jit.parse.finish()` returns now the second table containing aborted
> traces. Each table key is a trace number containing an array of
> potentially traces with this number, which was aborted.
>
> Needed for tarantool/tarantool#9924
> ---
>  .../unit-jit-parse-abort.test.lua             | 38 +++++++++++++++++++
>  test/tarantool-tests/utils/jit/parse.lua      | 22 ++++++++---
>  2 files changed, 55 insertions(+), 5 deletions(-)
>  create mode 100644 test/tarantool-tests/unit-jit-parse-abort.test.lua
>
> diff --git a/test/tarantool-tests/unit-jit-parse-abort.test.lua b/test/tarantool-tests/unit-jit-parse-abort.test.lua
> new file mode 100644
> index 00000000..91af5a56
> --- /dev/null
> +++ b/test/tarantool-tests/unit-jit-parse-abort.test.lua
<snipped>

> > diff --git a/test/tarantool-tests/utils/jit/parse.lua b/test/tarantool-tests/utils/jit/parse.lua
> index bcef5b35..0ce7f7c8 100644
> --- a/test/tarantool-tests/utils/jit/parse.lua
> +++ b/test/tarantool-tests/utils/jit/parse.lua
> @@ -22,6 +22,7 @@ local function trace_new(n)
>      parent = nil,
>      parent_exitno = nil,
>      is_stitched = false,
> +    abort_reason = nil,
>      start_loc = nil,
>      bc = {},
>      ir = {},
> @@ -87,9 +88,17 @@ local header_handlers = {
>      ctx.parsing_trace = nil
>      ctx.parsing = nil
>    end,
> -  abort = function(ctx, trace_num)
> +  abort = function(ctx, trace_num, line)
>      local traces = ctx.traces
>      assert(ctx.parsing_trace == trace_num)
> +
> +    local aborted_traces = ctx.aborted_traces
> +    if not aborted_traces[trace_num] then
> +      aborted_traces[trace_num] = {}
> +    end
> +    traces[trace_num].abort_reason = line:match('-- (.+)$')
Please drop a comment explaining the regexp.
> +    table.insert(aborted_traces[trace_num], traces[trace_num])
> +
>      ctx.parsing_trace = nil
>      ctx.parsing = nil
>      traces[trace_num] = nil
> @@ -137,11 +146,14 @@ end
>  local JDUMP_FILE
>
>  local function parse_jit_dump()
> -  local ctx = {traces = {}}
> +  local ctx = {
> +    aborted_traces = {},
> +    traces = {},
> +  }
>    for line in io.lines(JDUMP_FILE) do
>      parse_line(ctx, line)
>    end
> -  return ctx.traces
> +  return ctx.traces, ctx.aborted_traces
>  end
>
>  -- Start `jit.dump()` utility with the given flags, saving the
> @@ -167,10 +179,10 @@ M.finish = function()
>    -- Enable traces compilation for `jit.dump` back.
>    jit.on(jdump.on, true)
>    jit.on(jdump.off, true)
> -  local traces = parse_jit_dump()
> +  local traces, aborted_traces = parse_jit_dump()
>    os.remove(JDUMP_FILE)
>    JDUMP_FILE = nil
> -  return traces
> +  return traces, aborted_traces
>  end
>
>  -- Turn off compilation for the module to avoid side effects.
> --
> 2.44.0
>

Re: [Tarantool-patches] [PATCH luajit v1 3/5] test: allow `jit.parse` to return aborted traces

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the review!
Added a comment as you suggested and force-pushed the branch.

On 05.05.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a single nit below.
> On Mon, Apr 22, 2024 at 11:49:56AM UTC, Sergey Kaplun wrote:

<snipped>

> > @@ -87,9 +88,17 @@ local header_handlers = {
> >      ctx.parsing_trace = nil
> >      ctx.parsing = nil
> >    end,
> > -  abort = function(ctx, trace_num)
> > +  abort = function(ctx, trace_num, line)
> >      local traces = ctx.traces
> >      assert(ctx.parsing_trace == trace_num)
> > +
> > +    local aborted_traces = ctx.aborted_traces
> > +    if not aborted_traces[trace_num] then
> > +      aborted_traces[trace_num] = {}
> > +    end
> > +    traces[trace_num].abort_reason = line:match('-- (.+)$')
> Please drop a comment explaining the regexp.

Added:

===================================================================
diff --git a/test/tarantool-tests/utils/jit/parse.lua b/test/tarantool-tests/utils/jit/parse.lua
index 0ce7f7c8..f853437d 100644
--- a/test/tarantool-tests/utils/jit/parse.lua
+++ b/test/tarantool-tests/utils/jit/parse.lua
@@ -96,6 +96,8 @@ local header_handlers = {
     if not aborted_traces[trace_num] then
       aborted_traces[trace_num] = {}
     end
+    -- The reason is mentioned after "-- " at the end of the
+    -- string.
     traces[trace_num].abort_reason = line:match('-- (.+)$')
     table.insert(aborted_traces[trace_num], traces[trace_num])
 
===================================================================

> > +    table.insert(aborted_traces[trace_num], traces[trace_num])
> > +

<snipped>

> >

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v1 3/5] test: allow `jit.parse` to return aborted traces

[Sergey Bronnikov via Tarantool-patches]

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

Sergey,


thanks for the patch! Please see my comments below.

On 22.04.2024 11:49, Sergey Kaplun wrote:
> Now information about the abort of the trace is saved in the
> `abort_reason` field of the corresponding structure. The
> `jit.parse.finish()` returns now the second table containing aborted
> traces. Each table key is a trace number containing an array of
> potentially traces with this number, which was aborted.
>
> Needed for tarantool/tarantool#9924
> ---
>   .../unit-jit-parse-abort.test.lua             | 38 +++++++++++++++++++
>   test/tarantool-tests/utils/jit/parse.lua      | 22 ++++++++---
>   2 files changed, 55 insertions(+), 5 deletions(-)
>   create mode 100644 test/tarantool-tests/unit-jit-parse-abort.test.lua
>
> diff --git a/test/tarantool-tests/unit-jit-parse-abort.test.lua b/test/tarantool-tests/unit-jit-parse-abort.test.lua
> new file mode 100644
> index 00000000..91af5a56
> --- /dev/null
> +++ b/test/tarantool-tests/unit-jit-parse-abort.test.lua
> @@ -0,0 +1,38 @@
> +local tap = require('tap')
> +local test = tap.test('unit-jit-parse'):skipcond({

Usually a name passed to `tap.test` matches to test file name,

but here it is not matched.

> +  ['Test requires JIT enabled'] = not jit.status(),
> +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> +})
> +
> +local jparse = require('utils').jit.parse
> +
> +-- XXX: Avoid other traces compilation due to hotcount collisions
> +-- for predictable results.
> +jit.off()
> +jit.flush()
> +
> +test:plan(1)
> +
> +jit.on()
> +-- We only need the abort reason in the test.
> +jparse.start('t')

I would add a comment with explanation what does 't' flag mean.

Feel free to ignore.

> +
> +-- XXX: A trace always has at least 3 IR constants: for `nil`,
> +-- `false`, and `true`. Always fails to record with the set
> +-- `maxirconst` limit.
> +jit.opt.start('hotloop=1', 'maxirconst=1')
> +
> +for _ = 1, 3 do end
> +
> +local _, aborted_traces = jparse.finish()
> +
> +jit.off()
> +
> +assert(aborted_traces and aborted_traces[1], 'aborted trace is persisted')
> +
> +-- We tried to compile only one trace.
> +local reason = aborted_traces[1][1].abort_reason
> +
> +test:like(reason, 'trace too long', 'abort reason is correct')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/utils/jit/parse.lua b/test/tarantool-tests/utils/jit/parse.lua
> index bcef5b35..0ce7f7c8 100644
> --- a/test/tarantool-tests/utils/jit/parse.lua
> +++ b/test/tarantool-tests/utils/jit/parse.lua
> @@ -22,6 +22,7 @@ local function trace_new(n)
>       parent = nil,
>       parent_exitno = nil,
>       is_stitched = false,
> +    abort_reason = nil,
>       start_loc = nil,
>       bc = {},
>       ir = {},
> @@ -87,9 +88,17 @@ local header_handlers = {
>       ctx.parsing_trace = nil
>       ctx.parsing = nil
>     end,
> -  abort = function(ctx, trace_num)
> +  abort = function(ctx, trace_num, line)
>       local traces = ctx.traces
>       assert(ctx.parsing_trace == trace_num)
> +
> +    local aborted_traces = ctx.aborted_traces
> +    if not aborted_traces[trace_num] then
> +      aborted_traces[trace_num] = {}
> +    end
> +    traces[trace_num].abort_reason = line:match('-- (.+)$')
> +    table.insert(aborted_traces[trace_num], traces[trace_num])
> +
>       ctx.parsing_trace = nil
>       ctx.parsing = nil
>       traces[trace_num] = nil
> @@ -137,11 +146,14 @@ end
>   local JDUMP_FILE
>   
>   local function parse_jit_dump()
> -  local ctx = {traces = {}}
> +  local ctx = {
> +    aborted_traces = {},
> +    traces = {},
> +  }
>     for line in io.lines(JDUMP_FILE) do
>       parse_line(ctx, line)
>     end
> -  return ctx.traces
> +  return ctx.traces, ctx.aborted_traces
>   end
>   
>   -- Start `jit.dump()` utility with the given flags, saving the
> @@ -167,10 +179,10 @@ M.finish = function()
>     -- Enable traces compilation for `jit.dump` back.
>     jit.on(jdump.on, true)
>     jit.on(jdump.off, true)
> -  local traces = parse_jit_dump()
> +  local traces, aborted_traces = parse_jit_dump()
>     os.remove(JDUMP_FILE)
>     JDUMP_FILE = nil
> -  return traces
> +  return traces, aborted_traces
>   end
>   
>   -- Turn off compilation for the module to avoid side effects.

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

Re: [Tarantool-patches] [PATCH luajit v1 3/5] test: allow `jit.parse` to return aborted traces

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!

On 06.06.24, Sergey Bronnikov wrote:
> Sergey,
> 
> 
> thanks for the patch! Please see my comments below.
> 
> On 22.04.2024 11:49, Sergey Kaplun wrote:
> > Now information about the abort of the trace is saved in the
> > `abort_reason` field of the corresponding structure. The
> > `jit.parse.finish()` returns now the second table containing aborted
> > traces. Each table key is a trace number containing an array of
> > potentially traces with this number, which was aborted.
> >
> > Needed for tarantool/tarantool#9924
> > ---
> >   .../unit-jit-parse-abort.test.lua             | 38 +++++++++++++++++++
> >   test/tarantool-tests/utils/jit/parse.lua      | 22 ++++++++---
> >   2 files changed, 55 insertions(+), 5 deletions(-)
> >   create mode 100644 test/tarantool-tests/unit-jit-parse-abort.test.lua
> >
> > diff --git a/test/tarantool-tests/unit-jit-parse-abort.test.lua b/test/tarantool-tests/unit-jit-parse-abort.test.lua
> > new file mode 100644
> > index 00000000..91af5a56
> > --- /dev/null
> > +++ b/test/tarantool-tests/unit-jit-parse-abort.test.lua
> > @@ -0,0 +1,38 @@
> > +local tap = require('tap')
> > +local test = tap.test('unit-jit-parse'):skipcond({
> 
> Usually a name passed to `tap.test` matches to test file name,
> 
> but here it is not matched.

My bad, thanks!
Fixed, see the iterative patch below.

===================================================================
diff --git a/test/tarantool-tests/unit-jit-parse-abort.test.lua b/test/tarantool-tests/unit-jit-parse-abort.test.lua
index 91af5a56..f09e696c 100644
--- a/test/tarantool-tests/unit-jit-parse-abort.test.lua
+++ b/test/tarantool-tests/unit-jit-parse-abort.test.lua
@@ -1,5 +1,5 @@
 local tap = require('tap')
-local test = tap.test('unit-jit-parse'):skipcond({
+local test = tap.test('unit-jit-parse-abort'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
   ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
 })
===================================================================

> 
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> > +})
> > +
> > +local jparse = require('utils').jit.parse
> > +
> > +-- XXX: Avoid other traces compilation due to hotcount collisions
> > +-- for predictable results.
> > +jit.off()
> > +jit.flush()
> > +
> > +test:plan(1)
> > +
> > +jit.on()
> > +-- We only need the abort reason in the test.
> > +jparse.start('t')
> 
> I would add a comment with explanation what does 't' flag mean.
> 
> Feel free to ignore.

I suppose that readers who use `jit.dump` from time to time already
know all flags. If not, it can be easily found in <jit/dump.lua>.

> 
> > +

<snipped>

> >   -- Turn off compilation for the module to avoid side effects.

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v1 3/5] test: allow `jit.parse` to return aborted traces

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey

thanks for the fix and the answer. LGTM

On 13.06.2024 13:00, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
>
> On 06.06.24, Sergey Bronnikov wrote:
>> Sergey,
>>
>>
>> thanks for the patch! Please see my comments below.
>>
>> On 22.04.2024 11:49, Sergey Kaplun wrote:
>>> Now information about the abort of the trace is saved in the
>>> `abort_reason` field of the corresponding structure. The
>>> `jit.parse.finish()` returns now the second table containing aborted
>>> traces. Each table key is a trace number containing an array of
>>> potentially traces with this number, which was aborted.
>>>
>>> Needed for tarantool/tarantool#9924
>>> ---
>>>    .../unit-jit-parse-abort.test.lua             | 38 +++++++++++++++++++
>>>    test/tarantool-tests/utils/jit/parse.lua      | 22 ++++++++---
>>>    2 files changed, 55 insertions(+), 5 deletions(-)
>>>    create mode 100644 test/tarantool-tests/unit-jit-parse-abort.test.lua
>>>
>>> diff --git a/test/tarantool-tests/unit-jit-parse-abort.test.lua b/test/tarantool-tests/unit-jit-parse-abort.test.lua
>>> new file mode 100644
>>> index 00000000..91af5a56
>>> --- /dev/null
>>> +++ b/test/tarantool-tests/unit-jit-parse-abort.test.lua
>>> @@ -0,0 +1,38 @@
>>> +local tap = require('tap')
>>> +local test = tap.test('unit-jit-parse'):skipcond({
>> Usually a name passed to `tap.test` matches to test file name,
>>
>> but here it is not matched.
> My bad, thanks!
> Fixed, see the iterative patch below.
>
> ===================================================================
> diff --git a/test/tarantool-tests/unit-jit-parse-abort.test.lua b/test/tarantool-tests/unit-jit-parse-abort.test.lua
> index 91af5a56..f09e696c 100644
> --- a/test/tarantool-tests/unit-jit-parse-abort.test.lua
> +++ b/test/tarantool-tests/unit-jit-parse-abort.test.lua
> @@ -1,5 +1,5 @@
>   local tap = require('tap')
> -local test = tap.test('unit-jit-parse'):skipcond({
> +local test = tap.test('unit-jit-parse-abort'):skipcond({
>     ['Test requires JIT enabled'] = not jit.status(),
>     ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>   })
> ===================================================================
Thanks!
>>> +  ['Test requires JIT enabled'] = not jit.status(),
>>> +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>>> +})
>>> +
>>> +local jparse = require('utils').jit.parse
>>> +
>>> +-- XXX: Avoid other traces compilation due to hotcount collisions
>>> +-- for predictable results.
>>> +jit.off()
>>> +jit.flush()
>>> +
>>> +test:plan(1)
>>> +
>>> +jit.on()
>>> +-- We only need the abort reason in the test.
>>> +jparse.start('t')
>> I would add a comment with explanation what does 't' flag mean.
>>
>> Feel free to ignore.
> I suppose that readers who use `jit.dump` from time to time already
> know all flags. If not, it can be easily found in <jit/dump.lua>.
okay, let's keep it as is
>>> +
> <snipped>
>
>>>    -- Turn off compilation for the module to avoid side effects.

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

[Tarantool-patches] [PATCH luajit v1 4/5] Handle all types of errors during trace stitching.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Sergey Kaplun and Peter Cawley.

(cherry picked from commit d06beb0480c5d1eb53b3343e78063950275aa281)

This commit is a follow-up for the commit
1b8216023d5a79814389f1c1affef27c15d9de27 ("Throw any errors before stack
changes in trace stitching."). The patch prepends failures for the
specific error to be thrown. Nevertheless, the error may be thrown due
to retrying trace recording in the case when table bump optimization
is enabled or when OOM is observed during reallocation of the snapshot
or IR buffers.

This patch adds the corresponding protected frame and rethrows the error
after a fixup of the stack.

This patch also tests the correctness of copying the error message to
the top of the stack to get a valid "abort" reason in the `jit.dump`
utility.

Also, this patch fixes a non-ASCII space character in the comment for
<lj-720-errors-before-stitch.test.lua>.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9924
---
 src/lj_ffrecord.c                             | 21 ++++++--
 test/tarantool-tests/CMakeLists.txt           |  1 +
 .../lj-1166-error-stitch-oom-ir-buff.test.lua | 46 ++++++++++++++++
 ...j-1166-error-stitch-oom-snap-buff.test.lua | 54 +++++++++++++++++++
 .../lj-1166-error-stitch-table-bump.test.lua  | 38 +++++++++++++
 .../lj-1166-error-stitch/CMakeLists.txt       |  1 +
 .../lj-1166-error-stitch/mockalloc.c          | 51 ++++++++++++++++++
 .../lj-720-errors-before-stitch.test.lua      | 40 +++++++++++++-
 8 files changed, 245 insertions(+), 7 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
 create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
 create mode 100644 test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
 create mode 100644 test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
 create mode 100644 test/tarantool-tests/lj-1166-error-stitch/mockalloc.c

diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
index e3ed80fb..ff14e9e4 100644
--- a/src/lj_ffrecord.c
+++ b/src/lj_ffrecord.c
@@ -96,6 +96,14 @@ static ptrdiff_t results_wanted(jit_State *J)
     return -1;
 }
 
+static TValue *rec_stop_stitch_cp(lua_State *L, lua_CFunction dummy, void *ud)
+{
+  jit_State *J = (jit_State *)ud;
+  lj_record_stop(J, LJ_TRLINK_STITCH, 0);
+  UNUSED(L); UNUSED(dummy);
+  return NULL;
+}
+
 /* Trace stitching: add continuation below frame to start a new trace. */
 static void recff_stitch(jit_State *J)
 {
@@ -106,10 +114,7 @@ static void recff_stitch(jit_State *J)
   TValue *nframe = base + 1 + LJ_FR2;
   const BCIns *pc = frame_pc(base-1);
   TValue *pframe = frame_prevl(base-1);
-
-  /* Check for this now. Throwing in lj_record_stop messes up the stack. */
-  if (J->cur.nsnap >= (MSize)J->param[JIT_P_maxsnap])
-    lj_trace_err(J, LJ_TRERR_SNAPOV);
+  int errcode;
 
   /* Move func + args up in Lua stack and insert continuation. */
   memmove(&base[1], &base[-1-LJ_FR2], sizeof(TValue)*nslot);
@@ -134,13 +139,19 @@ static void recff_stitch(jit_State *J)
   J->baseslot += 2 + LJ_FR2;
   J->framedepth++;
 
-  lj_record_stop(J, LJ_TRLINK_STITCH, 0);
+  errcode = lj_vm_cpcall(L, NULL, J, rec_stop_stitch_cp);
 
   /* Undo Lua stack changes. */
   memmove(&base[-1-LJ_FR2], &base[1], sizeof(TValue)*nslot);
   setframe_pc(base-1, pc);
   L->base -= 2 + LJ_FR2;
   L->top -= 2 + LJ_FR2;
+
+  if (errcode) {
+    if (errcode == LUA_ERRRUN)
+      copyTV(L, L->top-1, L->top + (1 + LJ_FR2));
+    lj_err_throw(L, errcode);  /* Propagate errors. */
+  }
 }
 
 /* Fallback handler for fast functions that are not recorded (yet). */
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 56660932..d7c96078 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -39,6 +39,7 @@ add_subdirectory(lj-802-panic-at-mcode-protfail)
 add_subdirectory(lj-flush-on-trace)
 add_subdirectory(lj-1004-oom-error-frame)
 add_subdirectory(lj-1066-fix-cur_L-after-coroutine-resume)
+add_subdirectory(lj-1166-error-stitch)
 
 # The part of the memory profiler toolchain is located in tools
 # directory, jit, profiler, and bytecode toolchains are located
diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
new file mode 100644
index 00000000..e3a5397d
--- /dev/null
+++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
@@ -0,0 +1,46 @@
+local tap = require('tap')
+
+-- Test file to demonstrate unbalanced Lua stack after instruction
+-- recording due to throwing an error at recording of a stitched
+-- function.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1166.
+
+local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
+})
+
+test:plan(1)
+
+local mockalloc = require('mockalloc')
+
+local function create_chunk(n_slots)
+  local chunk = ''
+  for i = 1, n_slots do
+    chunk = chunk .. ('local s%d\n'):format(i)
+  end
+  chunk = chunk .. 'for i = 1, 2 do\n'
+  -- Generate additional IR instructions.
+  for i = 1, n_slots do
+    chunk = chunk .. ('  s%d = i + %d\n'):format(i, i)
+  end
+  -- `math.modf()` recording is NYI.
+  chunk = chunk .. '  math.modf(1)\n'
+  chunk = chunk .. 'end\n'
+  return chunk
+end
+
+-- XXX: amount of slots is empirical.
+local tracef = assert(loadstring(create_chunk(175)))
+
+jit.opt.start('hotloop=1', '-loop', '-fold')
+
+mockalloc.mock()
+
+tracef()
+
+mockalloc.unmock()
+
+test:ok(true, 'stack is balanced')
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
new file mode 100644
index 00000000..8d671f8d
--- /dev/null
+++ b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
@@ -0,0 +1,54 @@
+local tap = require('tap')
+
+-- Test file to demonstrate unbalanced Lua stack after instruction
+-- recording due to throwing an error at recording of a stitched
+-- function.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1166.
+
+local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
+})
+
+test:plan(1)
+
+local mockalloc = require('mockalloc')
+
+local function create_chunk(n_conds)
+  local chunk = ''
+  chunk = chunk .. 'for i = 1, 2 do\n'
+  -- Each condition adds additional snapshot.
+  for i = 1, n_conds do
+    chunk = chunk .. ('  if i < %d then end\n'):format(i + n_conds)
+  end
+  -- `math.modf()` recording is NYI.
+  chunk = chunk .. '  math.modf(1)\n'
+  chunk = chunk .. 'end\n'
+  return chunk
+end
+
+-- XXX: Need to compile the cycle in the `create_chunk()` to
+-- preallocate the snapshot buffer.
+jit.opt.start('hotloop=1', '-loop', '-fold')
+
+-- XXX: Amount of slots is empirical.
+local tracef = assert(loadstring(create_chunk(6)))
+
+-- XXX: Remove previous trace.
+jit.off()
+jit.flush()
+
+-- XXX: Update hotcounts to avoid hash collisions.
+jit.opt.start('hotloop=1')
+
+jit.on()
+
+mockalloc.mock()
+
+tracef()
+
+mockalloc.unmock()
+
+test:ok(true, 'stack is balanced')
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
new file mode 100644
index 00000000..f2453bbe
--- /dev/null
+++ b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
@@ -0,0 +1,38 @@
+local tap = require('tap')
+
+-- Test file to demonstrate unbalanced Lua stack after instruction
+-- recording due to throwing an error at recording of a stitched
+-- function. The test fails with LUAJIT_ENABLE_TABLE_BUMP enabled.
+-- See also:
+-- * https://github.com/LuaJIT/LuaJIT/issues/606,
+-- * https://github.com/LuaJIT/LuaJIT/issues/1166.
+
+local test = tap.test('lj-1166-error-stitch-table-bump'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+-- `math.modf` recording is NYI.
+-- Local `modf` simplifies `jit.dump()` output.
+local modf = math.modf
+
+jit.opt.start('hotloop=1')
+
+-- luacheck: no unused
+local t
+-- There is no need to run the trace itself. Just check the
+-- correctness of a recording.
+for i = 1, 2 do
+  t = {}
+  -- Cause table rehashing to trigger table bump optimization.
+  t[i] = i
+  -- Forcify stitch. This will throw an error at the end of
+  -- recording, since trace recording should be retried after
+  -- bytecode updating.
+  modf(1)
+end
+
+test:ok(true, 'stack is balanced')
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt b/test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
new file mode 100644
index 00000000..1ebf253b
--- /dev/null
+++ b/test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
@@ -0,0 +1 @@
+BuildTestCLib(mockalloc mockalloc.c)
diff --git a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
new file mode 100644
index 00000000..d6d3492e
--- /dev/null
+++ b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
@@ -0,0 +1,51 @@
+#include "lua.h"
+#include "lauxlib.h"
+
+#undef NDEBUG
+#include <assert.h>
+
+static lua_Alloc old_allocf = NULL;
+static void *old_alloc_state = NULL;
+
+/* Function to be used instead of the default allocator. */
+static void *mock_allocf(void *ud, void *ptr, size_t osize, size_t nsize)
+{
+	assert(old_allocf != NULL);
+	/*
+	 * Check the specific reallocation related to the IR
+	 * buffer or the snapshot buffer.
+	 */
+	if (osize * 2 == nsize)
+		return NULL;
+	return old_allocf(ud, ptr, osize, nsize);
+}
+
+static int mock(lua_State *L)
+{
+	assert(old_allocf == NULL);
+	old_allocf = lua_getallocf(L, &old_alloc_state);
+	lua_setallocf(L, mock_allocf, old_alloc_state);
+	return 0;
+}
+
+static int unmock(lua_State *L)
+{
+	assert(old_allocf != NULL);
+	assert(old_allocf != mock_allocf);
+	lua_setallocf(L, old_allocf, old_alloc_state);
+	old_allocf = NULL;
+	old_alloc_state = NULL;
+	return 0;
+}
+
+static const struct luaL_Reg mockalloc[] = {
+	{"mock", mock},
+	{"unmock", unmock},
+	{NULL, NULL}
+};
+
+LUA_API int luaopen_mockalloc(lua_State *L)
+{
+	luaL_register(L, "mockalloc", mockalloc);
+	return 1;
+}
diff --git a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
index d750b721..6e8f70c2 100644
--- a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
+++ b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
@@ -1,13 +1,27 @@
 local tap = require('tap')
 local test = tap.test('lj-720-errors-before-stitch'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
+  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
 })
-test:plan(1)
 
--- `math.modf` recording is NYI.
+local jparse = require('utils').jit.parse
+
+-- `math.modf` recording is NYI.
 -- Local `modf` simplifies `jit.dump()` output.
 local modf = math.modf
+
+-- XXX: Avoid other traces compilation due to hotcount collisions
+-- for predictable results.
+jit.off()
+jit.flush()
+
+test:plan(2)
+
+-- We only need the abort reason in the test.
+jparse.start('t')
+
 jit.opt.start('hotloop=1', 'maxsnap=1')
+jit.on()
 
 -- The loop has only two iterations: the first to detect its
 -- hotness and the second to record it. The snapshot limit is
@@ -17,5 +31,27 @@ for _ = 1, 2 do
   modf(1.2)
 end
 
+local _, aborted_traces = jparse.finish()
+
+jit.off()
+
 test:ok(true, 'stack is balanced')
+
+-- Tarantool may compile traces on the startup. These traces
+-- already exceed the maximum snapshot amount we set after they
+-- are compiled. Hence, there is no need to reallocate the
+-- snapshot buffer, so the check for the snap size is not
+-- triggered.
+test:skipcond({
+  -- luacheck: no global
+  ['Impossible to predict the number of snapshots for Tarantool'] = _TARANTOOL,
+})
+
+assert(aborted_traces and aborted_traces[1], 'aborted trace is persisted')
+
+-- We tried to compile only one trace.
+local reason = aborted_traces[1][1].abort_reason
+
+test:like(reason, 'too many snapshots', 'abort reason is correct')
+
 test:done(true)
-- 
2.44.0

Re: [Tarantool-patches] [PATCH luajit v1 4/5] Handle all types of errors during trace stitching.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM

Re: [Tarantool-patches] [PATCH luajit v1 4/5] Handle all types of errors during trace stitching.

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey,


thanks for the patch! Please see my comments.

On 22.04.2024 11:49, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun and Peter Cawley.
>
> (cherry picked from commit d06beb0480c5d1eb53b3343e78063950275aa281)
>
> This commit is a follow-up for the commit
> 1b8216023d5a79814389f1c1affef27c15d9de27 ("Throw any errors before stack
> changes in trace stitching."). The patch prepends failures for the
> specific error to be thrown. Nevertheless, the error may be thrown due
> to retrying trace recording in the case when table bump optimization
> is enabled or when OOM is observed during reallocation of the snapshot
> or IR buffers.
>
> This patch adds the corresponding protected frame and rethrows the error
> after a fixup of the stack.
>
> This patch also tests the correctness of copying the error message to
> the top of the stack to get a valid "abort" reason in the `jit.dump`
> utility.
>
> Also, this patch fixes a non-ASCII space character in the comment for
> <lj-720-errors-before-stitch.test.lua>.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9924
> ---
>   src/lj_ffrecord.c                             | 21 ++++++--
>   test/tarantool-tests/CMakeLists.txt           |  1 +
>   .../lj-1166-error-stitch-oom-ir-buff.test.lua | 46 ++++++++++++++++
>   ...j-1166-error-stitch-oom-snap-buff.test.lua | 54 +++++++++++++++++++
>   .../lj-1166-error-stitch-table-bump.test.lua  | 38 +++++++++++++
>   .../lj-1166-error-stitch/CMakeLists.txt       |  1 +
>   .../lj-1166-error-stitch/mockalloc.c          | 51 ++++++++++++++++++
>   .../lj-720-errors-before-stitch.test.lua      | 40 +++++++++++++-
>   8 files changed, 245 insertions(+), 7 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
>   create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
>   create mode 100644 test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
>   create mode 100644 test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
>   create mode 100644 test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
>
> diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
> index e3ed80fb..ff14e9e4 100644
> --- a/src/lj_ffrecord.c
> +++ b/src/lj_ffrecord.c
> @@ -96,6 +96,14 @@ static ptrdiff_t results_wanted(jit_State *J)
>       return -1;
>   }
>   
> +static TValue *rec_stop_stitch_cp(lua_State *L, lua_CFunction dummy, void *ud)
> +{
> +  jit_State *J = (jit_State *)ud;
> +  lj_record_stop(J, LJ_TRLINK_STITCH, 0);
> +  UNUSED(L); UNUSED(dummy);
> +  return NULL;
> +}
> +
>   /* Trace stitching: add continuation below frame to start a new trace. */
>   static void recff_stitch(jit_State *J)
>   {
> @@ -106,10 +114,7 @@ static void recff_stitch(jit_State *J)
>     TValue *nframe = base + 1 + LJ_FR2;
>     const BCIns *pc = frame_pc(base-1);
>     TValue *pframe = frame_prevl(base-1);
> -
> -  /* Check for this now. Throwing in lj_record_stop messes up the stack. */
> -  if (J->cur.nsnap >= (MSize)J->param[JIT_P_maxsnap])
> -    lj_trace_err(J, LJ_TRERR_SNAPOV);
> +  int errcode;
>   
>     /* Move func + args up in Lua stack and insert continuation. */
>     memmove(&base[1], &base[-1-LJ_FR2], sizeof(TValue)*nslot);
> @@ -134,13 +139,19 @@ static void recff_stitch(jit_State *J)
>     J->baseslot += 2 + LJ_FR2;
>     J->framedepth++;
>   
> -  lj_record_stop(J, LJ_TRLINK_STITCH, 0);
> +  errcode = lj_vm_cpcall(L, NULL, J, rec_stop_stitch_cp);
>   
>     /* Undo Lua stack changes. */
>     memmove(&base[-1-LJ_FR2], &base[1], sizeof(TValue)*nslot);
>     setframe_pc(base-1, pc);
>     L->base -= 2 + LJ_FR2;
>     L->top -= 2 + LJ_FR2;
> +
> +  if (errcode) {
> +    if (errcode == LUA_ERRRUN)
> +      copyTV(L, L->top-1, L->top + (1 + LJ_FR2));
> +    lj_err_throw(L, errcode);  /* Propagate errors. */
> +  }
>   }
>   
>   /* Fallback handler for fast functions that are not recorded (yet). */
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 56660932..d7c96078 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -39,6 +39,7 @@ add_subdirectory(lj-802-panic-at-mcode-protfail)
>   add_subdirectory(lj-flush-on-trace)
>   add_subdirectory(lj-1004-oom-error-frame)
>   add_subdirectory(lj-1066-fix-cur_L-after-coroutine-resume)
> +add_subdirectory(lj-1166-error-stitch)
>   
>   # The part of the memory profiler toolchain is located in tools
>   # directory, jit, profiler, and bytecode toolchains are located
> diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> new file mode 100644
> index 00000000..e3a5397d
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> @@ -0,0 +1,46 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate unbalanced Lua stack after instruction
> +-- recording due to throwing an error at recording of a stitched
> +-- function.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1166.
> +
> +local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({

should a name in tap.test match to test file name?

now it is not.

> +  ['Test requires JIT enabled'] = not jit.status(),
> +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> +})
> +
> +test:plan(1)
> +
> +local mockalloc = require('mockalloc')
> +
> +local function create_chunk(n_slots)

I would add a comment like this:


--- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
+++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
@@ -14,6 +14,18 @@ test:plan(1)

  local mockalloc = require('mockalloc')

+-- Generate a Lua chunk like below:
+-- local s1
+-- local s2
+-- ...
+-- local sN
+-- for i = 1, 2 do
+--   s1 = i + 1
+--   s2 = i + 2
+--   ...
+--   sN = i + N
+--   math.modf(1)
+-- end
  local function create_chunk(n_slots)
    local chunk = ''
    for i = 1, n_slots do


> +  local chunk = ''
> +  for i = 1, n_slots do
> +    chunk = chunk .. ('local s%d\n'):format(i)
> +  end
> +  chunk = chunk .. 'for i = 1, 2 do\n'
> +  -- Generate additional IR instructions.
> +  for i = 1, n_slots do
> +    chunk = chunk .. ('  s%d = i + %d\n'):format(i, i)
> +  end
> +  -- `math.modf()` recording is NYI.
> +  chunk = chunk .. '  math.modf(1)\n'
> +  chunk = chunk .. 'end\n'
> +  return chunk
> +end
> +
> +-- XXX: amount of slots is empirical.
> +local tracef = assert(loadstring(create_chunk(175)))
> +
> +jit.opt.start('hotloop=1', '-loop', '-fold')
> +
> +mockalloc.mock()
> +
> +tracef()
> +
> +mockalloc.unmock()
> +
> +test:ok(true, 'stack is balanced')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> new file mode 100644
> index 00000000..8d671f8d
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> @@ -0,0 +1,54 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate unbalanced Lua stack after instruction
> +-- recording due to throwing an error at recording of a stitched
> +-- function.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1166.
> +
> +local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> +})
> +
> +test:plan(1)
> +
> +local mockalloc = require('mockalloc')
> +
> +local function create_chunk(n_conds)
the same as above: please add a comment with an example of generated Lua 
chunk
> +  local chunk = ''
> +  chunk = chunk .. 'for i = 1, 2 do\n'
> +  -- Each condition adds additional snapshot.
> +  for i = 1, n_conds do
> +    chunk = chunk .. ('  if i < %d then end\n'):format(i + n_conds)
> +  end
> +  -- `math.modf()` recording is NYI.
> +  chunk = chunk .. '  math.modf(1)\n'
> +  chunk = chunk .. 'end\n'
> +  return chunk
> +end
> +
> +-- XXX: Need to compile the cycle in the `create_chunk()` to
> +-- preallocate the snapshot buffer.
> +jit.opt.start('hotloop=1', '-loop', '-fold')
> +
> +-- XXX: Amount of slots is empirical.
> +local tracef = assert(loadstring(create_chunk(6)))
> +
> +-- XXX: Remove previous trace.
> +jit.off()
> +jit.flush()
> +
> +-- XXX: Update hotcounts to avoid hash collisions.
> +jit.opt.start('hotloop=1')
> +
> +jit.on()
> +
> +mockalloc.mock()
> +
> +tracef()
> +
> +mockalloc.unmock()
> +
> +test:ok(true, 'stack is balanced')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
> new file mode 100644
> index 00000000..f2453bbe
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua

this test is not failed after reverting patch


> @@ -0,0 +1,38 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate unbalanced Lua stack after instruction
> +-- recording due to throwing an error at recording of a stitched
> +-- function. The test fails with LUAJIT_ENABLE_TABLE_BUMP enabled.
> +-- See also:
> +-- *https://github.com/LuaJIT/LuaJIT/issues/606,
> +-- *https://github.com/LuaJIT/LuaJIT/issues/1166.
> +
> +local test = tap.test('lj-1166-error-stitch-table-bump'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +-- `math.modf` recording is NYI.
> +-- Local `modf` simplifies `jit.dump()` output.
> +local modf = math.modf
> +
> +jit.opt.start('hotloop=1')
> +
> +-- luacheck: no unused
> +local t
> +-- There is no need to run the trace itself. Just check the
> +-- correctness of a recording.
> +for i = 1, 2 do
> +  t = {}
> +  -- Cause table rehashing to trigger table bump optimization.
> +  t[i] = i
> +  -- Forcify stitch. This will throw an error at the end of
> +  -- recording, since trace recording should be retried after
> +  -- bytecode updating.
> +  modf(1)
> +end
> +
> +test:ok(true, 'stack is balanced')
> +
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt b/test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
> new file mode 100644
> index 00000000..1ebf253b
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
> @@ -0,0 +1 @@
> +BuildTestCLib(mockalloc mockalloc.c)
> diff --git a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
> new file mode 100644
> index 00000000..d6d3492e
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
> @@ -0,0 +1,51 @@
> +#include "lua.h"
> +#include "lauxlib.h"
> +
> +#undef NDEBUG
> +#include <assert.h>
> +
> +static lua_Alloc old_allocf = NULL;
> +static void *old_alloc_state = NULL;
> +
> +/* Function to be used instead of the default allocator. */
> +static void *mock_allocf(void *ud, void *ptr, size_t osize, size_t nsize)
> +{
> +	assert(old_allocf != NULL);
> +	/*
> +	 * Check the specific reallocation related to the IR
> +	 * buffer or the snapshot buffer.
> +	 */
> +	if (osize * 2 == nsize)
> +		return NULL;
> +	return old_allocf(ud, ptr, osize, nsize);
> +}
> +
> +static int mock(lua_State *L)


It is actually not a test mock.

According to definition [1] test mock imitate a behavior of a real object.

Your memory allocator behaves as a real allocator, but in some cases it 
will return

a NULL instead of memory address. What if we rename "mock" to "allocator 
with fault injection"?


1. https://www.martinfowler.com/articles/mocksArentStubs.html

> +{
> +	assert(old_allocf == NULL);
> +	old_allocf = lua_getallocf(L, &old_alloc_state);
> +	lua_setallocf(L, mock_allocf, old_alloc_state);
> +	return 0;
> +}
> +
> +static int unmock(lua_State *L)
> +{
> +	assert(old_allocf != NULL);
> +	assert(old_allocf != mock_allocf);
> +	lua_setallocf(L, old_allocf, old_alloc_state);
> +	old_allocf = NULL;
> +	old_alloc_state = NULL;
> +	return 0;
> +}
> +
> +static const struct luaL_Reg mockalloc[] = {
> +	{"mock", mock},
> +	{"unmock", unmock},
> +	{NULL, NULL}
> +};
> +
> +LUA_API int luaopen_mockalloc(lua_State *L)
> +{
> +	luaL_register(L, "mockalloc", mockalloc);
> +	return 1;
> +}
> diff --git a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
> index d750b721..6e8f70c2 100644
> --- a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
> +++ b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
> @@ -1,13 +1,27 @@
>   local tap = require('tap')
>   local test = tap.test('lj-720-errors-before-stitch'):skipcond({
>     ['Test requires JIT enabled'] = not jit.status(),
> +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>   })
> -test:plan(1)
>   
> --- `math.modf` recording is NYI.
> +local jparse = require('utils').jit.parse
> +
> +-- `math.modf` recording is NYI.
>   -- Local `modf` simplifies `jit.dump()` output.
>   local modf = math.modf
> +
> +-- XXX: Avoid other traces compilation due to hotcount collisions
> +-- for predictable results.
> +jit.off()
> +jit.flush()
> +
> +test:plan(2)
> +
> +-- We only need the abort reason in the test.
> +jparse.start('t')
> +
>   jit.opt.start('hotloop=1', 'maxsnap=1')
> +jit.on()
>   
>   -- The loop has only two iterations: the first to detect its
>   -- hotness and the second to record it. The snapshot limit is
> @@ -17,5 +31,27 @@ for _ = 1, 2 do
>     modf(1.2)
>   end
>   
> +local _, aborted_traces = jparse.finish()
> +
> +jit.off()
> +
>   test:ok(true, 'stack is balanced')
> +
> +-- Tarantool may compile traces on the startup. These traces
> +-- already exceed the maximum snapshot amount we set after they
> +-- are compiled. Hence, there is no need to reallocate the
> +-- snapshot buffer, so the check for the snap size is not
> +-- triggered.
> +test:skipcond({
> +  -- luacheck: no global
> +  ['Impossible to predict the number of snapshots for Tarantool'] = _TARANTOOL,
> +})
> +
> +assert(aborted_traces and aborted_traces[1], 'aborted trace is persisted')
> +
> +-- We tried to compile only one trace.
> +local reason = aborted_traces[1][1].abort_reason
> +
> +test:like(reason, 'too many snapshots', 'abort reason is correct')
> +
>   test:done(true)

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

Re: [Tarantool-patches] [PATCH luajit v1 4/5] Handle all types of errors during trace stitching.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Please consider my answers below.

On 06.06.24, Sergey Bronnikov wrote:
> Hi, Sergey,
> 
> 
> thanks for the patch! Please see my comments.
> 
> On 22.04.2024 11:49, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Sergey Kaplun and Peter Cawley.
> >
> > (cherry picked from commit d06beb0480c5d1eb53b3343e78063950275aa281)
> >
> > This commit is a follow-up for the commit
> > 1b8216023d5a79814389f1c1affef27c15d9de27 ("Throw any errors before stack
> > changes in trace stitching."). The patch prepends failures for the
> > specific error to be thrown. Nevertheless, the error may be thrown due
> > to retrying trace recording in the case when table bump optimization
> > is enabled or when OOM is observed during reallocation of the snapshot
> > or IR buffers.
> >
> > This patch adds the corresponding protected frame and rethrows the error
> > after a fixup of the stack.
> >
> > This patch also tests the correctness of copying the error message to
> > the top of the stack to get a valid "abort" reason in the `jit.dump`
> > utility.
> >
> > Also, this patch fixes a non-ASCII space character in the comment for
> > <lj-720-errors-before-stitch.test.lua>.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9924
> > ---
> >   src/lj_ffrecord.c                             | 21 ++++++--
> >   test/tarantool-tests/CMakeLists.txt           |  1 +
> >   .../lj-1166-error-stitch-oom-ir-buff.test.lua | 46 ++++++++++++++++
> >   ...j-1166-error-stitch-oom-snap-buff.test.lua | 54 +++++++++++++++++++
> >   .../lj-1166-error-stitch-table-bump.test.lua  | 38 +++++++++++++
> >   .../lj-1166-error-stitch/CMakeLists.txt       |  1 +
> >   .../lj-1166-error-stitch/mockalloc.c          | 51 ++++++++++++++++++
> >   .../lj-720-errors-before-stitch.test.lua      | 40 +++++++++++++-
> >   8 files changed, 245 insertions(+), 7 deletions(-)
> >   create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> >   create mode 100644 test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> >   create mode 100644 test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
> >   create mode 100644 test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt
> >   create mode 100644 test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
> >
> > diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
> > index e3ed80fb..ff14e9e4 100644
> > --- a/src/lj_ffrecord.c
> > +++ b/src/lj_ffrecord.c

<snipped>

> > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> > index 56660932..d7c96078 100644
> > --- a/test/tarantool-tests/CMakeLists.txt
> > +++ b/test/tarantool-tests/CMakeLists.txt
> > @@ -39,6 +39,7 @@ add_subdirectory(lj-802-panic-at-mcode-protfail)
> >   add_subdirectory(lj-flush-on-trace)
> >   add_subdirectory(lj-1004-oom-error-frame)
> >   add_subdirectory(lj-1066-fix-cur_L-after-coroutine-resume)
> > +add_subdirectory(lj-1166-error-stitch)
> >   
> >   # The part of the memory profiler toolchain is located in tools
> >   # directory, jit, profiler, and bytecode toolchains are located
> > diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> > new file mode 100644
> > index 00000000..e3a5397d
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> > @@ -0,0 +1,46 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate unbalanced Lua stack after instruction
> > +-- recording due to throwing an error at recording of a stitched
> > +-- function.
> > +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1166.
> > +
> > +local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
> 
> should a name in tap.test match to test file name?
> 
> now it is not.

My mistake during copipasting. Fixed.

> 
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> > +})
> > +
> > +test:plan(1)
> > +
> > +local mockalloc = require('mockalloc')
> > +
> > +local function create_chunk(n_slots)
> 
> I would add a comment like this:
> 
> 
> --- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> @@ -14,6 +14,18 @@ test:plan(1)
> 
>   local mockalloc = require('mockalloc')
> 
> +-- Generate a Lua chunk like below:
> +-- local s1
> +-- local s2
> +-- ...
> +-- local sN
> +-- for i = 1, 2 do
> +--   s1 = i + 1
> +--   s2 = i + 2
> +--   ...
> +--   sN = i + N
> +--   math.modf(1)
> +-- end
>   local function create_chunk(n_slots)
>     local chunk = ''
>     for i = 1, n_slots do

Added, see the iterative patch below.

> 
> 
> > +  local chunk = ''

<snipped>

> > +
> > +local mockalloc = require('mockalloc')
> > +
> > +local function create_chunk(n_conds)
> the same as above: please add a comment with an example of generated Lua 
> chunk

Added, see the iterative patch below.

> > +  local chunk = ''
> > +  chunk = chunk .. 'for i = 1, 2 do\n'
> > +  -- Each condition adds additional snapshot.
> > +  for i = 1, n_conds do
> > +    chunk = chunk .. ('  if i < %d then end\n'):format(i + n_conds)
> > +  end
> > +  -- `math.modf()` recording is NYI.
> > +  chunk = chunk .. '  math.modf(1)\n'
> > +  chunk = chunk .. 'end\n'
> > +  return chunk
> > +end
> > +
> > +-- XXX: Need to compile the cycle in the `create_chunk()` to
> > +-- preallocate the snapshot buffer.
> > +jit.opt.start('hotloop=1', '-loop', '-fold')
> > +
> > +-- XXX: Amount of slots is empirical.
> > +local tracef = assert(loadstring(create_chunk(6)))
> > +
> > +-- XXX: Remove previous trace.
> > +jit.off()
> > +jit.flush()
> > +
> > +-- XXX: Update hotcounts to avoid hash collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +jit.on()
> > +
> > +mockalloc.mock()
> > +
> > +tracef()
> > +
> > +mockalloc.unmock()
> > +
> > +test:ok(true, 'stack is balanced')
> > +
> > +test:done(true)
> > diff --git a/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
> > new file mode 100644
> > index 00000000..f2453bbe
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
> 
> this test is not failed after reverting patch

Do you build luajit with -DLUAJIT_ENABLE_TABLE_BUMP=ON?

<snipped>

> > diff --git a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
> > new file mode 100644
> > index 00000000..d6d3492e
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
> > @@ -0,0 +1,51 @@

<snipped>

> > +
> > +/* Function to be used instead of the default allocator. */
> > +static void *mock_allocf(void *ud, void *ptr, size_t osize, size_t nsize)
> > +{
> > +	assert(old_allocf != NULL);
> > +	/*
> > +	 * Check the specific reallocation related to the IR
> > +	 * buffer or the snapshot buffer.
> > +	 */
> > +	if (osize * 2 == nsize)
> > +		return NULL;
> > +	return old_allocf(ud, ptr, osize, nsize);
> > +}
> > +
> > +static int mock(lua_State *L)
> 
> 
> It is actually not a test mock.
> 
> According to definition [1] test mock imitate a behavior of a real object.
> 
> Your memory allocator behaves as a real allocator, but in some cases it 
> will return
> 
> a NULL instead of memory address. What if we rename "mock" to "allocator 
> with fault injection"?

I renamed `mock_allocf` to `allocf_with_injection()` to avoid
confusing.

===================================================================
diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
index cf3ab0f5..eb229a5c 100644
--- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
+++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
@@ -5,7 +5,7 @@ local tap = require('tap')
 -- function.
 -- See also: https://github.com/LuaJIT/LuaJIT/issues/1166.
 
-local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
+local test = tap.test('lj-1166-error-stitch-oom-ir-buff'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
   ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
 })
@@ -22,6 +22,16 @@ jit.flush()
 
 test:plan(2)
 
+-- Generate the following Lua chunk:
+-- local s1
+-- ...
+-- local sN
+-- for i = 1, 2 do
+--   s1 = i + 1
+--   ...
+--   sN = i + N
+--   math.modf(1)
+-- end
 local function create_chunk(n_slots)
   local chunk = ''
   for i = 1, n_slots do
diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
index 8bbdd96b..8ae26386 100644
--- a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
+++ b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
@@ -20,6 +20,13 @@ jit.flush()
 
 test:plan(2)
 
+-- Generate the following Lua chunk:
+-- for i = 1, 2 do
+--   if i < 1 then end
+--   ...
+--   if i < N then end
+--   math.modf(1)
+-- end
 local function create_chunk(n_conds)
   local chunk = ''
   chunk = chunk .. 'for i = 1, 2 do\n'
diff --git a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
index d6d3492e..19d32f8b 100644
--- a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
+++ b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
@@ -8,7 +8,8 @@ static lua_Alloc old_allocf = NULL;
 static void *old_alloc_state = NULL;
 
 /* Function to be used instead of the default allocator. */
-static void *mock_allocf(void *ud, void *ptr, size_t osize, size_t nsize)
+static void *allocf_with_injection(void *ud, void *ptr, size_t osize,
+				   size_t nsize)
 {
 	assert(old_allocf != NULL);
 	/*
@@ -24,14 +25,14 @@ static int mock(lua_State *L)
 {
 	assert(old_allocf == NULL);
 	old_allocf = lua_getallocf(L, &old_alloc_state);
-	lua_setallocf(L, mock_allocf, old_alloc_state);
+	lua_setallocf(L, allocf_with_injection, old_alloc_state);
 	return 0;
 }
 
 static int unmock(lua_State *L)
 {
 	assert(old_allocf != NULL);
-	assert(old_allocf != mock_allocf);
+	assert(old_allocf != allocf_with_injection);
 	lua_setallocf(L, old_allocf, old_alloc_state);
 	old_allocf = NULL;
 	old_alloc_state = NULL;
===================================================================

> 
> 
> 1. https://www.martinfowler.com/articles/mocksArentStubs.html
> 
> > +{

<snipped>

> > diff --git a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
> > index d750b721..6e8f70c2 100644
> > --- a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
> > +++ b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v1 4/5] Handle all types of errors during trace stitching.

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey

thanks for the fixes and answers!

See my answers below.

On 13.06.2024 13:25, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Please consider my answers below.
>
> On 06.06.24, Sergey Bronnikov wrote:
<snipped>
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> @@ -0,0 +1,46 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate unbalanced Lua stack after instruction
> +-- recording due to throwing an error at recording of a stitched
> +-- function.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1166.
> +
> +local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
>> should a name in tap.test match to test file name?
>>
>> now it is not.
> My mistake during copipasting. Fixed.
Thanks!
>>> +  ['Test requires JIT enabled'] = not jit.status(),
>>> +  ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>>> +})
>>> +
>>> +test:plan(1)
>>> +
>>> +local mockalloc = require('mockalloc')
>>> +
>>> +local function create_chunk(n_slots)
>> I would add a comment like this:
>>
>>
>> --- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
>> +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
>> @@ -14,6 +14,18 @@ test:plan(1)
>>
>>    local mockalloc = require('mockalloc')
>>
>> +-- Generate a Lua chunk like below:
>> +-- local s1
>> +-- local s2
>> +-- ...
>> +-- local sN
>> +-- for i = 1, 2 do
>> +--   s1 = i + 1
>> +--   s2 = i + 2
>> +--   ...
>> +--   sN = i + N
>> +--   math.modf(1)
>> +-- end
>>    local function create_chunk(n_slots)
>>      local chunk = ''
>>      for i = 1, n_slots do
> Added, see the iterative patch below.

Thanks!


>
>>
>>> +  local chunk = ''
> <snipped>
>
>>> +
>>> +local mockalloc = require('mockalloc')
>>> +
>>> +local function create_chunk(n_conds)
>> the same as above: please add a comment with an example of generated Lua
>> chunk
> Added, see the iterative patch below.
>
Thanks!
> diff --git a/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
> new file mode 100644
> index 00000000..f2453bbe
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua
>> this test is not failed after reverting patch
> Do you build luajit with -DLUAJIT_ENABLE_TABLE_BUMP=ON?
>
> <snipped>
>
My bad. With enabled option tests failed:

The following tests FAILED:
         127 - 
test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua (SEGFAULT)
         128 - 
test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua (SEGFAULT)
         129 - 
test/tarantool-tests/lj-1166-error-stitch-table-bump.test.lua (SEGFAULT)
Errors while running CTest

>>> diff --git a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
>>> new file mode 100644
>>> index 00000000..d6d3492e
>>> --- /dev/null
>>> +++ b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
>>> @@ -0,0 +1,51 @@
> <snipped>
>
>>> +
>>> +/* Function to be used instead of the default allocator. */
>>> +static void *mock_allocf(void *ud, void *ptr, size_t osize, size_t nsize)
>>> +{
>>> +	assert(old_allocf != NULL);
>>> +	/*
>>> +	 * Check the specific reallocation related to the IR
>>> +	 * buffer or the snapshot buffer.
>>> +	 */
>>> +	if (osize * 2 == nsize)
>>> +		return NULL;
>>> +	return old_allocf(ud, ptr, osize, nsize);
>>> +}
>>> +
>>> +static int mock(lua_State *L)
>>
>> It is actually not a test mock.
>>
>> According to definition [1] test mock imitate a behavior of a real object.
>>
>> Your memory allocator behaves as a real allocator, but in some cases it
>> will return
>>
>> a NULL instead of memory address. What if we rename "mock" to "allocator
>> with fault injection"?
> I renamed `mock_allocf` to `allocf_with_injection()` to avoid
> confusing.
>
> ===================================================================
> diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> index cf3ab0f5..eb229a5c 100644
> --- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> @@ -5,7 +5,7 @@ local tap = require('tap')
>   -- function.
>   -- See also:https://github.com/LuaJIT/LuaJIT/issues/1166.
>   
> -local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
> +local test = tap.test('lj-1166-error-stitch-oom-ir-buff'):skipcond({
>     ['Test requires JIT enabled'] = not jit.status(),
>     ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>   })
> @@ -22,6 +22,16 @@ jit.flush()
>   
>   test:plan(2)
>   
> +-- Generate the following Lua chunk:
> +-- local s1
> +-- ...
> +-- local sN
> +-- for i = 1, 2 do
> +--   s1 = i + 1
> +--   ...
> +--   sN = i + N
> +--   math.modf(1)
> +-- end
>   local function create_chunk(n_slots)
>     local chunk = ''
>     for i = 1, n_slots do
> diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> index 8bbdd96b..8ae26386 100644
> --- a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> @@ -20,6 +20,13 @@ jit.flush()
>   
>   test:plan(2)
>   
> +-- Generate the following Lua chunk:
> +-- for i = 1, 2 do
> +--   if i < 1 then end
> +--   ...
> +--   if i < N then end
> +--   math.modf(1)
> +-- end
>   local function create_chunk(n_conds)
>     local chunk = ''
>     chunk = chunk .. 'for i = 1, 2 do\n'
> diff --git a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
> index d6d3492e..19d32f8b 100644
> --- a/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
> +++ b/test/tarantool-tests/lj-1166-error-stitch/mockalloc.c
> @@ -8,7 +8,8 @@ static lua_Alloc old_allocf = NULL;
>   static void *old_alloc_state = NULL;
>   
>   /* Function to be used instead of the default allocator. */
> -static void *mock_allocf(void *ud, void *ptr, size_t osize, size_t nsize)
> +static void *allocf_with_injection(void *ud, void *ptr, size_t osize,
> +				   size_t nsize)
>   {
>   	assert(old_allocf != NULL);
>   	/*
> @@ -24,14 +25,14 @@ static int mock(lua_State *L)
>   {
>   	assert(old_allocf == NULL);
>   	old_allocf = lua_getallocf(L, &old_alloc_state);
> -	lua_setallocf(L, mock_allocf, old_alloc_state);
> +	lua_setallocf(L, allocf_with_injection, old_alloc_state);
>   	return 0;
>   }
>   
>   static int unmock(lua_State *L)
>   {
>   	assert(old_allocf != NULL);
> -	assert(old_allocf != mock_allocf);
> +	assert(old_allocf != allocf_with_injection);
>   	lua_setallocf(L, old_allocf, old_alloc_state);
>   	old_allocf = NULL;
>   	old_alloc_state = NULL;
> ===================================================================
>
Filename and other variables still use prefix "mock":

[0] ~/sources/MRG/tarantool/third_party/luajit$ grep -R mock test
test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua:local 
mockalloc = require('mockalloc')
test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua:mockalloc.mock()
test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua:mockalloc.unmock()
test/tarantool-tests/CMakeLists.txt:# Some tests use `LD_PRELOAD` to 
mock system calls (like
test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua:local 
mockalloc = require('mockalloc')
test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua:mockalloc.mock()
test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua:mockalloc.unmock()
test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:static int 
mock(lua_State *L)
test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:static int 
unmock(lua_State *L)
test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:static const 
struct luaL_Reg mockalloc[] = {
test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:  {"mock", mock},
test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:  {"unmock", unmock},
test/tarantool-tests/lj-1166-error-stitch/mockalloc.c:LUA_API int 
luaopen_mockalloc(lua_State *L)
test/tarantool-tests/lj-1166-error-stitch/mockalloc.c: luaL_register(L, 
"mockalloc", mockalloc);
test/tarantool-tests/lj-1166-error-stitch/CMakeLists.txt:BuildTestCLib(mockalloc 
mockalloc.c)

have you left it intentionally?

>>
>> 1.https://www.martinfowler.com/articles/mocksArentStubs.html
>>
>>> +{
> <snipped>
>
>>> diff --git a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
>>> index d750b721..6e8f70c2 100644
>>> --- a/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
>>> +++ b/test/tarantool-tests/lj-720-errors-before-stitch.test.lua
> <snipped>
>

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

[Tarantool-patches] [PATCH luajit v1 5/5] Use generic trace error for OOM during trace stitching.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Sergey Kaplun.

(cherry picked from commit b8b49bf3954b23e32e34187a6ada00021c26e172)

The previous commit doesn't handle the case when the error code is
`LUA_ERRMEM`. This patch adds a workaround by using the generic error
message.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9924
---
 src/lj_ffrecord.c                             |  2 +
 .../lj-1166-error-stitch-oom-ir-buff.test.lua | 41 ++++++++++++++++++-
 ...j-1166-error-stitch-oom-snap-buff.test.lua | 37 +++++++++++++++--
 3 files changed, 75 insertions(+), 5 deletions(-)

diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
index ff14e9e4..d5fc081e 100644
--- a/src/lj_ffrecord.c
+++ b/src/lj_ffrecord.c
@@ -150,6 +150,8 @@ static void recff_stitch(jit_State *J)
   if (errcode) {
     if (errcode == LUA_ERRRUN)
       copyTV(L, L->top-1, L->top + (1 + LJ_FR2));
+    else
+      setintV(L->top-1, (int32_t)LJ_TRERR_RECERR);
     lj_err_throw(L, errcode);  /* Propagate errors. */
   }
 }
diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
index e3a5397d..cf3ab0f5 100644
--- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
+++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
@@ -10,10 +10,18 @@ local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
   ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
 })
 
-test:plan(1)
-
+local jparse = require('utils').jit.parse
 local mockalloc = require('mockalloc')
 
+local IS_DUALNUM = tostring(tonumber('-0')) ~= tostring(-0)
+
+-- XXX: Avoid other traces compilation due to hotcount collisions
+-- for predictable results.
+jit.off()
+jit.flush()
+
+test:plan(2)
+
 local function create_chunk(n_slots)
   local chunk = ''
   for i = 1, n_slots do
@@ -33,6 +41,10 @@ end
 -- XXX: amount of slots is empirical.
 local tracef = assert(loadstring(create_chunk(175)))
 
+-- We only need the abort reason in the test.
+jparse.start('t')
+
+jit.on()
 jit.opt.start('hotloop=1', '-loop', '-fold')
 
 mockalloc.mock()
@@ -41,6 +53,31 @@ tracef()
 
 mockalloc.unmock()
 
+local _, aborted_traces = jparse.finish()
+
+jit.off()
+
 test:ok(true, 'stack is balanced')
 
+-- Tarantool may compile traces on the startup. These traces
+-- already exceed the maximum IR amount before the trace in this
+-- test is compiled. Hence, there is no need to reallocate the IR
+-- buffer, so the check for the IR size is not triggered.
+test:skipcond({
+  -- luacheck: no global
+  ['Impossible to predict the number of IRs for Tarantool'] = _TARANTOOL,
+  -- The amount of IR for traces is different for non x86/x64
+  -- arches and DUALNUM mode.
+  ['Disabled for non-x86_64 arches'] = jit.arch ~= 'x64' and jit.arch ~= 'x86',
+  ['Disabled for DUALNUM mode'] = IS_DUALNUM,
+})
+
+assert(aborted_traces and aborted_traces[1], 'aborted trace is persisted')
+
+-- We tried to compile only one trace.
+local reason = aborted_traces[1][1].abort_reason
+
+test:like(reason, 'error thrown or hook called during recording',
+          'abort reason is correct')
+
 test:done(true)
diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
index 8d671f8d..8bbdd96b 100644
--- a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
+++ b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
@@ -10,10 +10,16 @@ local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
   ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
 })
 
-test:plan(1)
-
+local jparse = require('utils').jit.parse
 local mockalloc = require('mockalloc')
 
+-- XXX: Avoid other traces compilation due to hotcount collisions
+-- for predictable results.
+jit.off()
+jit.flush()
+
+test:plan(2)
+
 local function create_chunk(n_conds)
   local chunk = ''
   chunk = chunk .. 'for i = 1, 2 do\n'
@@ -27,6 +33,7 @@ local function create_chunk(n_conds)
   return chunk
 end
 
+jit.on()
 -- XXX: Need to compile the cycle in the `create_chunk()` to
 -- preallocate the snapshot buffer.
 jit.opt.start('hotloop=1', '-loop', '-fold')
@@ -38,9 +45,11 @@ local tracef = assert(loadstring(create_chunk(6)))
 jit.off()
 jit.flush()
 
+-- We only need the abort reason in the test.
+jparse.start('t')
+
 -- XXX: Update hotcounts to avoid hash collisions.
 jit.opt.start('hotloop=1')
-
 jit.on()
 
 mockalloc.mock()
@@ -49,6 +58,28 @@ tracef()
 
 mockalloc.unmock()
 
+local _, aborted_traces = jparse.finish()
+
+jit.off()
+
 test:ok(true, 'stack is balanced')
 
+-- Tarantool may compile traces on the startup. These traces
+-- already exceed the maximum snapshot amount before the trace in
+-- this test is compiled. Hence, there is no need to reallocate
+-- the snapshot buffer, so the check for the snap size is not
+-- triggered.
+test:skipcond({
+  -- luacheck: no global
+  ['Impossible to predict the number of snapshots for Tarantool'] = _TARANTOOL,
+})
+
+assert(aborted_traces and aborted_traces[1], 'aborted trace is persisted')
+
+-- We tried to compile only one trace.
+local reason = aborted_traces[1][1].abort_reason
+
+test:like(reason, 'error thrown or hook called during recording',
+          'abort reason is correct')
+
 test:done(true)
-- 
2.44.0

Re: [Tarantool-patches] [PATCH luajit v1 5/5] Use generic trace error for OOM during trace stitching.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM

Re: [Tarantool-patches] [PATCH luajit v1 5/5] Use generic trace error for OOM during trace stitching.

[Sergey Bronnikov via Tarantool-patches]

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

Sergey,


thanks for the patch! See my comments below.


Sergey

On 22.04.2024 11:49, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun.
>
> (cherry picked from commit b8b49bf3954b23e32e34187a6ada00021c26e172)
>
> The previous commit doesn't handle the case when the error code is
> `LUA_ERRMEM`. This patch adds a workaround by using the generic error
> message.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#9924
> ---
>   src/lj_ffrecord.c                             |  2 +
>   .../lj-1166-error-stitch-oom-ir-buff.test.lua | 41 ++++++++++++++++++-
>   ...j-1166-error-stitch-oom-snap-buff.test.lua | 37 +++++++++++++++--
>   3 files changed, 75 insertions(+), 5 deletions(-)
>
> diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
> index ff14e9e4..d5fc081e 100644
> --- a/src/lj_ffrecord.c
> +++ b/src/lj_ffrecord.c
> @@ -150,6 +150,8 @@ static void recff_stitch(jit_State *J)
>     if (errcode) {
>       if (errcode == LUA_ERRRUN)
>         copyTV(L, L->top-1, L->top + (1 + LJ_FR2));
> +    else
> +      setintV(L->top-1, (int32_t)LJ_TRERR_RECERR);
>       lj_err_throw(L, errcode);  /* Propagate errors. */
>     }
>   }
> diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> index e3a5397d..cf3ab0f5 100644
> --- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> @@ -10,10 +10,18 @@ local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
>     ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>   })
>   
> -test:plan(1)
> -
> +local jparse = require('utils').jit.parse
>   local mockalloc = require('mockalloc')
>   
> +local IS_DUALNUM = tostring(tonumber('-0')) ~= tostring(-0)
> +
> +-- XXX: Avoid other traces compilation due to hotcount collisions
> +-- for predictable results.
> +jit.off()
> +jit.flush()
> +
> +test:plan(2)
> +
>   local function create_chunk(n_slots)
>     local chunk = ''
>     for i = 1, n_slots do
> @@ -33,6 +41,10 @@ end
>   -- XXX: amount of slots is empirical.
>   local tracef = assert(loadstring(create_chunk(175)))
>   
> +-- We only need the abort reason in the test.
> +jparse.start('t')
> +
> +jit.on()
>   jit.opt.start('hotloop=1', '-loop', '-fold')
>   
>   mockalloc.mock()
> @@ -41,6 +53,31 @@ tracef()
>   
>   mockalloc.unmock()
>   
> +local _, aborted_traces = jparse.finish()
> +
> +jit.off()
> +
>   test:ok(true, 'stack is balanced')
>   
> +-- Tarantool may compile traces on the startup. These traces
> +-- already exceed the maximum IR amount before the trace in this
> +-- test is compiled. Hence, there is no need to reallocate the IR
> +-- buffer, so the check for the IR size is not triggered.
> +test:skipcond({
> +  -- luacheck: no global

I made a patch that remove inline suppressions [1].

I propose to merge it and remove inline suppressions in your patch 
series too.


[1]: 
https://lists.tarantool.org/tarantool-patches/88eab16fca9056a057df5506a0af637c8d4a0ffd.1717682341.git.sergeyb@tarantool.org/T/#u

> +  ['Impossible to predict the number of IRs for Tarantool'] = _TARANTOOL,
> +  -- The amount of IR for traces is different for non x86/x64
> +  -- arches and DUALNUM mode.
> +  ['Disabled for non-x86_64 arches'] = jit.arch ~= 'x64' and jit.arch ~= 'x86',
> +  ['Disabled for DUALNUM mode'] = IS_DUALNUM,
> +})
> +
> +assert(aborted_traces and aborted_traces[1], 'aborted trace is persisted')
> +
> +-- We tried to compile only one trace.
> +local reason = aborted_traces[1][1].abort_reason
> +
> +test:like(reason, 'error thrown or hook called during recording',
> +          'abort reason is correct')
> +
>   test:done(true)
> diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> index 8d671f8d..8bbdd96b 100644
> --- a/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-snap-buff.test.lua
> @@ -10,10 +10,16 @@ local test = tap.test('lj-1166-error-stitch-oom-snap-buff'):skipcond({
>     ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
>   })
>   
> -test:plan(1)
> -
> +local jparse = require('utils').jit.parse
>   local mockalloc = require('mockalloc')
>   
> +-- XXX: Avoid other traces compilation due to hotcount collisions
> +-- for predictable results.
> +jit.off()
> +jit.flush()
> +
> +test:plan(2)
> +
>   local function create_chunk(n_conds)
>     local chunk = ''
>     chunk = chunk .. 'for i = 1, 2 do\n'
> @@ -27,6 +33,7 @@ local function create_chunk(n_conds)
>     return chunk
>   end
>   
> +jit.on()
>   -- XXX: Need to compile the cycle in the `create_chunk()` to
>   -- preallocate the snapshot buffer.
>   jit.opt.start('hotloop=1', '-loop', '-fold')
> @@ -38,9 +45,11 @@ local tracef = assert(loadstring(create_chunk(6)))
>   jit.off()
>   jit.flush()
>   
> +-- We only need the abort reason in the test.
> +jparse.start('t')
Same comment as in previous mail - let's add a comment regarding 't'.
> +
>   -- XXX: Update hotcounts to avoid hash collisions.
>   jit.opt.start('hotloop=1')
> -
>   jit.on()
>   
>   mockalloc.mock()
> @@ -49,6 +58,28 @@ tracef()
>   
>   mockalloc.unmock()
Same comment as in previous mail - let's avoid name 'mock' here.
>   
> +local _, aborted_traces = jparse.finish()
> +
> +jit.off()
> +
>   test:ok(true, 'stack is balanced')
>   
> +-- Tarantool may compile traces on the startup. These traces
> +-- already exceed the maximum snapshot amount before the trace in
> +-- this test is compiled. Hence, there is no need to reallocate
> +-- the snapshot buffer, so the check for the snap size is not
> +-- triggered.
> +test:skipcond({
> +  -- luacheck: no global
> +  ['Impossible to predict the number of snapshots for Tarantool'] = _TARANTOOL,
> +})
> +
> +assert(aborted_traces and aborted_traces[1], 'aborted trace is persisted')
> +
> +-- We tried to compile only one trace.
> +local reason = aborted_traces[1][1].abort_reason
> +
> +test:like(reason, 'error thrown or hook called during recording',
> +          'abort reason is correct')
> +
>   test:done(true)

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

Re: [Tarantool-patches] [PATCH luajit v1 5/5] Use generic trace error for OOM during trace stitching.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Please consider my answers below.

On 06.06.24, Sergey Bronnikov wrote:
> Sergey,
> 
> 
> thanks for the patch! See my comments below.
> 
> 
> Sergey
> 
> On 22.04.2024 11:49, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Sergey Kaplun.
> >
> > (cherry picked from commit b8b49bf3954b23e32e34187a6ada00021c26e172)
> >
> > The previous commit doesn't handle the case when the error code is
> > `LUA_ERRMEM`. This patch adds a workaround by using the generic error
> > message.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9924
> > ---
> >   src/lj_ffrecord.c                             |  2 +
> >   .../lj-1166-error-stitch-oom-ir-buff.test.lua | 41 ++++++++++++++++++-
> >   ...j-1166-error-stitch-oom-snap-buff.test.lua | 37 +++++++++++++++--
> >   3 files changed, 75 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
> > index ff14e9e4..d5fc081e 100644
> > --- a/src/lj_ffrecord.c
> > +++ b/src/lj_ffrecord.c
> > @@ -150,6 +150,8 @@ static void recff_stitch(jit_State *J)
> >     if (errcode) {
> >       if (errcode == LUA_ERRRUN)
> >         copyTV(L, L->top-1, L->top + (1 + LJ_FR2));
> > +    else
> > +      setintV(L->top-1, (int32_t)LJ_TRERR_RECERR);
> >       lj_err_throw(L, errcode);  /* Propagate errors. */
> >     }
> >   }
> > diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> > index e3a5397d..cf3ab0f5 100644
> > --- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
> > +++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua

<snipped>

> > +test:skipcond({
> > +  -- luacheck: no global
> 
> I made a patch that remove inline suppressions [1].
> 
> I propose to merge it and remove inline suppressions in your patch 
> series too.

Ok, I'll remove these supressions after merging your patch and rebasing
to the master.

<snipped>

> >   
> > +-- We only need the abort reason in the test.
> > +jparse.start('t')
> Same comment as in previous mail - let's add a comment regarding 't'.

See my answers here [1].

> > +
> >   -- XXX: Update hotcounts to avoid hash collisions.
> >   jit.opt.start('hotloop=1')
> > -
> >   jit.on()
> >   
> >   mockalloc.mock()
> > @@ -49,6 +58,28 @@ tracef()
> >   
> >   mockalloc.unmock()
> Same comment as in previous mail - let's avoid name 'mock' here.

But we actually mock the allocator here, don't we?
Thus, I renamed the new allocated function to avoid confusion.
If you insist, please suggest the correct name instead `mock` |
`unmock`.

> >   

<snipped>

> >   test:done(true)

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2024-June/029218.html

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v1 5/5] Use generic trace error for OOM during trace stitching.

[Sergey Bronnikov via Tarantool-patches]

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

Hi, Sergey

thanks for the answers.

On 13.06.2024 13:31, Sergey Kaplun wrote:

<snipped>

> +test:skipcond({
>>> +  -- luacheck: no global
>> I made a patch that remove inline suppressions [1].
>>
>> I propose to merge it and remove inline suppressions in your patch
>> series too.
> Ok, I'll remove these supressions after merging your patch and rebasing
> to the master.
Okay!
> <snipped>
>
>>>    
>>> +-- We only need the abort reason in the test.
>>> +jparse.start('t')
>> Same comment as in previous mail - let's add a comment regarding 't'.
> See my answers here [1].
Okay!
>
>>> +
>>>    -- XXX: Update hotcounts to avoid hash collisions.
>>>    jit.opt.start('hotloop=1')
>>> -
>>>    jit.on()
>>>    
>>>    mockalloc.mock()
>>> @@ -49,6 +58,28 @@ tracef()
>>>    
>>>    mockalloc.unmock()
>> Same comment as in previous mail - let's avoid name 'mock' here.
> But we actually mock the allocator here, don't we?
> Thus, I renamed the new allocated function to avoid confusion.
> If you insist, please suggest the correct name instead `mock` |
> `unmock`.

We can replace "mock"/"unmock" to "{enable, disable} error injection".

Actually, I don't want to insist. I'm ok if it wouldn't confuse anyone.

>>>    
> <snipped>
>
>>>    test:done(true)
> [1]:https://lists.tarantool.org/pipermail/tarantool-patches/2024-June/029218.html
>

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

Re: [Tarantool-patches] [PATCH luajit v1 5/5] Use generic trace error for OOM during trace stitching.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!

On 13.06.24, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for the answers.

<snipped>

> >>>    mockalloc.unmock()
> >> Same comment as in previous mail - let's avoid name 'mock' here.
> > But we actually mock the allocator here, don't we?
> > Thus, I renamed the new allocated function to avoid confusion.
> > If you insist, please suggest the correct name instead `mock` |
> > `unmock`.
> 
> We can replace "mock"/"unmock" to "{enable, disable} error injection".
> 
> Actually, I don't want to insist. I'm ok if it wouldn't confuse anyone.

Replaced in the 2nd verison of the patch-set [1].

> 
> >>>    
> > <snipped>
> >
> >>>    test:done(true)
> > [1]:https://lists.tarantool.org/pipermail/tarantool-patches/2024-June/029218.html
> >

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2024-June/029242.html

-- 
Best regards,
Sergey Kaplun