[Tarantool-patches] [PATCH luajit 0/3] Dualnumber mode fixes

[Tarantool-patches] [PATCH luajit 0/3] Dualnumber mode fixes

[Sergey Kaplun via Tarantool-patches]

Something strange happened with the previous cover-letter, so resend
it.

The first 2 patches:
* sysprof: fix interval parsing in dual-number mode
* ci: introduce workflow for exotic builds
are needed to introduce workflow for dualnumber mode build, to test the
3rd commit:
* x86/x64: Fix loop realignment.

The first and second commits are separated as far as only the first one
is needed to backport to tarantool-1.10 branch.

PR: https://github.com/tarantool/tarantool/pull/8180
Issues:
* https://github.com/LuaJIT/LuaJIT/issues/556
* https://github.com/tarantool/tarantool/issues/8069
Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-556-fix-loop-realignment-full-ci

Mike Pall (1):
  x86/x64: Fix loop realignment.

Sergey Kaplun (2):
  sysprof: fix interval parsing in dual-number mode
  ci: introduce workflow for exotic builds

 .github/workflows/exotic-builds-testing.yml   | 71 +++++++++++++++++++
 src/lib_misc.c                                |  2 +-
 src/lj_asm_x86.h                              |  5 +-
 .../lj-556-fix-loop-realignment.test.lua      | 18 +++++
 4 files changed, 93 insertions(+), 3 deletions(-)
 create mode 100644 .github/workflows/exotic-builds-testing.yml
 create mode 100644 test/tarantool-tests/lj-556-fix-loop-realignment.test.lua

-- 
2.34.1

[Tarantool-patches] [PATCH luajit 1/3] sysprof: fix interval parsing in dual-number mode

[Sergey Kaplun via Tarantool-patches]

When the profiling interval is parsed, `tvisnum()` is used to check that
the given value is a number. But in case of dual-number mode this check
returns false as far as the given value is an integer, so `tvisnumber()`
should be used to cover both cases.
---
 src/lib_misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lib_misc.c b/src/lib_misc.c
index 06fb9f9f..c18d297e 100644
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -209,7 +209,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
   {
     cTValue *interval = lj_tab_getstr(options, lj_str_newlit(L, "interval"));
     opt->interval = SYSPROF_DEFAULT_INTERVAL;
-    if (interval && tvisnum(interval)) {
+    if (interval && tvisnumber(interval)) {
       int32_t signed_interval = numberVint(interval);
       if (signed_interval < 1)
         return PROFILE_ERRUSE;
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 1/3] sysprof: fix interval parsing in dual-number mode

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>When the profiling interval is parsed, `tvisnum()` is used to check that
>>the given value is a number. But in case of dual-number mode this check
>>returns false as far as the given value is an integer, so `tvisnumber()`
>>should be used to cover both cases.
>>---
>> src/lib_misc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/src/lib_misc.c b/src/lib_misc.c
>>index 06fb9f9f..c18d297e 100644
>>--- a/src/lib_misc.c
>>+++ b/src/lib_misc.c
>>@@ -209,7 +209,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>>   {
>>     cTValue *interval = lj_tab_getstr(options, lj_str_newlit(L, "interval"));
>>     opt->interval = SYSPROF_DEFAULT_INTERVAL;
>>- if (interval && tvisnum(interval)) {
>>+ if (interval && tvisnumber(interval)) {
>>       int32_t signed_interval = numberVint(interval);
>>       if (signed_interval < 1)
>>         return PROFILE_ERRUSE;
>>--
>>2.34.1
> 

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

Re: [Tarantool-patches] [PATCH luajit 1/3] sysprof: fix interval parsing in dual-number mode

[sergos via Tarantool-patches]

Hi!

Just a nit in message, LGTM.

Sergos

> On 18 Jan 2023, at 23:16, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> When the profiling interval is parsed, `tvisnum()` is used to check that
       a
  
> the given value is a number. But in case of dual-number mode this check
> returns false as far as the given value is an integer, so `tvisnumber()`
> should be used to cover both cases.
> ---
> src/lib_misc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/lib_misc.c b/src/lib_misc.c
> index 06fb9f9f..c18d297e 100644
> --- a/src/lib_misc.c
> +++ b/src/lib_misc.c
> @@ -209,7 +209,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in
>   {
>     cTValue *interval = lj_tab_getstr(options, lj_str_newlit(L, "interval"));
>     opt->interval = SYSPROF_DEFAULT_INTERVAL;
> -    if (interval && tvisnum(interval)) {
> +    if (interval && tvisnumber(interval)) {
>       int32_t signed_interval = numberVint(interval);
>       if (signed_interval < 1)
>         return PROFILE_ERRUSE;
> -- 
> 2.34.1
> 

Re: [Tarantool-patches] [PATCH luajit 1/3] sysprof: fix interval parsing in dual-number mode

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!
Thanks for the review!

On 26.01.23, sergos wrote:
> Hi!
> 
> Just a nit in message, LGTM.
> 
> Sergos
> 
> > On 18 Jan 2023, at 23:16, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > When the profiling interval is parsed, `tvisnum()` is used to check that
>        a

Fixed, thanks!

>   
> > the given value is a number. But in case of dual-number mode this check
> > returns false as far as the given value is an integer, so `tvisnumber()`
> > should be used to cover both cases.
> > ---

<snipped>

> 

-- 
Best regards,
Sergey Kaplun

[Tarantool-patches] [PATCH luajit 2/3] ci: introduce workflow for exotic builds

[Sergey Kaplun via Tarantool-patches]

This workflow is well-suited for test specific builds like dual-number
mode on x86_x64, build with disabled JIT or FFI, etc.

For now, just the dual-number mode is added, due to the need to test
future commits. If there is need to add a new build type `FLAVOR` column
of CI matrix should be exposed. Also, just Linux x86_64 is tested,
because we want to decrease resource usage in CI.
---
 .github/workflows/exotic-builds-testing.yml | 71 +++++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 .github/workflows/exotic-builds-testing.yml

diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
new file mode 100644
index 00000000..cd0c14d7
--- /dev/null
+++ b/.github/workflows/exotic-builds-testing.yml
@@ -0,0 +1,71 @@
+name: "Exotic builds testing"
+
+on:
+  push:
+    branches-ignore:
+      - '**-notest'
+      - 'upstream-**'
+    tags-ignore:
+      - '**'
+
+concurrency:
+  # An update of a developer branch cancels the previously
+  # scheduled workflow run for this branch. However, the default
+  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
+  # etc.) workflow runs are never canceled.
+  #
+  # We use a trick here: define the concurrency group as 'workflow
+  # run ID' + # 'workflow run attempt' because it is a unique
+  # combination for any run. So it effectively discards grouping.
+  #
+  # XXX: we cannot use `github.sha` as a unique identifier because
+  # pushing a tag may cancel a run that works on a branch push
+  # event.
+  group: ${{ (
+    github.ref == 'refs/heads/tarantool' ||
+    startsWith(github.ref, 'refs/heads/tarantool-')) &&
+    format('{0}-{1}', github.run_id, github.run_attempt) ||
+    format('{0}-{1}', github.workflow, github.ref) }}
+  cancel-in-progress: true
+
+jobs:
+  test-x86_64-exotic:
+    strategy:
+      fail-fast: false
+      matrix:
+        BUILDTYPE: [Debug, Release]
+        GC64: [ON, OFF]
+        FLAVOR: [dualnum]
+        include:
+          - BUILDTYPE: Debug
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
+          - BUILDTYPE: Release
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
+          - FLAVOR: dualnum
+            FLAVORFLAGS: -DLUAJIT_NUMMODE=2
+    runs-on: [self-hosted, regular, Linux, x86_64]
+    name: >
+      LuaJIT ${{ matrix.FLAVOR }}
+      (Linux/x86_64)
+      ${{ matrix.BUILDTYPE }}
+      GC64:${{ matrix.GC64 }}
+    steps:
+      - uses: actions/checkout@v2.3.4
+        with:
+          fetch-depth: 0
+          submodules: recursive
+      - name: setup Linux
+        uses: ./.github/actions/setup-linux
+      - name: configure
+        run: >
+          cmake -S . -B ${{ env.BUILDDIR }}
+          -G Ninja
+          ${{ matrix.CMAKEFLAGS }}
+          ${{ matrix.FLAVORFLAGS }}
+          -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
+      - name: build
+        run: cmake --build . --parallel
+        working-directory: ${{ env.BUILDDIR }}
+      - name: test
+        run: cmake --build . --parallel --target test
+        working-directory: ${{ env.BUILDDIR }}
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 2/3] ci: introduce workflow for exotic builds

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>This workflow is well-suited for test specific builds like dual-number
>>mode on x86_x64, build with disabled JIT or FFI, etc.
>>
>>For now, just the dual-number mode is added, due to the need to test
>>future commits. If there is need to add a new build type `FLAVOR` column
>>of CI matrix should be exposed. Also, just Linux x86_64 is tested,
>>because we want to decrease resource usage in CI.
>>---
>> .github/workflows/exotic-builds-testing.yml | 71 +++++++++++++++++++++
>> 1 file changed, 71 insertions(+)
>> create mode 100644 .github/workflows/exotic-builds-testing.yml
>>
>>diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
>>new file mode 100644
>>index 00000000..cd0c14d7
>>--- /dev/null
>>+++ b/.github/workflows/exotic-builds-testing.yml
>>@@ -0,0 +1,71 @@
>>+name: "Exotic builds testing"
>>+
>>+on:
>>+ push:
>>+ branches-ignore:
>>+ - '**-notest'
>>+ - 'upstream-**'
>>+ tags-ignore:
>>+ - '**'
>>+
>>+concurrency:
>>+ # An update of a developer branch cancels the previously
>>+ # scheduled workflow run for this branch. However, the default
>>+ # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
>>+ # etc.) workflow runs are never canceled.
>>+ #
>>+ # We use a trick here: define the concurrency group as 'workflow
>>+ # run ID' + # 'workflow run attempt' because it is a unique
>>+ # combination for any run. So it effectively discards grouping.
>>+ #
>>+ # XXX: we cannot use `github.sha` as a unique identifier because
>>+ # pushing a tag may cancel a run that works on a branch push
>>+ # event.
>>+ group: ${{ (
>>+ github.ref == 'refs/heads/tarantool' ||
>>+ startsWith(github.ref, 'refs/heads/tarantool-')) &&
>>+ format('{0}-{1}', github.run_id, github.run_attempt) ||
>>+ format('{0}-{1}', github.workflow, github.ref) }}
>>+ cancel-in-progress: true
>>+
>>+jobs:
>>+ test-x86_64-exotic:
>>+ strategy:
>>+ fail-fast: false
>>+ matrix:
>>+ BUILDTYPE: [Debug, Release]
>>+ GC64: [ON, OFF]
>>+ FLAVOR: [dualnum]
>>+ include:
>>+ - BUILDTYPE: Debug
>>+ CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
>>+ - BUILDTYPE: Release
>>+ CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
>>+ - FLAVOR: dualnum
>>+ FLAVORFLAGS: -DLUAJIT_NUMMODE=2
>>+ runs-on: [self-hosted, regular, Linux, x86_64]
>>+ name: >
>>+ LuaJIT ${{ matrix.FLAVOR }}
>>+ (Linux/x86_64)
>>+ ${{ matrix.BUILDTYPE }}
>>+ GC64:${{ matrix.GC64 }}
>>+ steps:
>>+ - uses: actions/checkout@v2.3.4
>>+ with:
>>+ fetch-depth: 0
>>+ submodules: recursive
>>+ - name: setup Linux
>>+ uses: ./.github/actions/setup-linux
>>+ - name: configure
>>+ run: >
>>+ cmake -S . -B ${{ env.BUILDDIR }}
>>+ -G Ninja
>>+ ${{ matrix.CMAKEFLAGS }}
>>+ ${{ matrix.FLAVORFLAGS }}
>>+ -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
>>+ - name: build
>>+ run: cmake --build . --parallel
>>+ working-directory: ${{ env.BUILDDIR }}
>>+ - name: test
>>+ run: cmake --build . --parallel --target test
>>+ working-directory: ${{ env.BUILDDIR }}
>>--
>>2.34.1
> 

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

Re: [Tarantool-patches] [PATCH luajit 2/3] ci: introduce workflow for exotic builds

[sergos via Tarantool-patches]

Hi!

Thanks for the patch!

Some question on build options.

Sergos

> On 18 Jan 2023, at 23:16, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> This workflow is well-suited for test specific builds like dual-number
> mode on x86_x64, build with disabled JIT or FFI, etc.
> 
> For now, just the dual-number mode is added, due to the need to test
> future commits. If there is need to add a new build type `FLAVOR` column
                             ^                            ^---------
                             a                           then(?) the

> of CI matrix should be exposed. Also, just Linux x86_64 is tested,
                        extended?      xxxxx                      ^ only

> because we want to decrease resource usage in CI.
> ---
> .github/workflows/exotic-builds-testing.yml | 71 +++++++++++++++++++++
> 1 file changed, 71 insertions(+)
> create mode 100644 .github/workflows/exotic-builds-testing.yml
> 
> diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
> new file mode 100644
> index 00000000..cd0c14d7
> --- /dev/null
> +++ b/.github/workflows/exotic-builds-testing.yml
> @@ -0,0 +1,71 @@
> +name: "Exotic builds testing"
> +
> +on:
> +  push:
> +    branches-ignore:
> +      - '**-notest'
> +      - 'upstream-**'
> +    tags-ignore:
> +      - '**'
> +
> +concurrency:
> +  # An update of a developer branch cancels the previously
> +  # scheduled workflow run for this branch. However, the default
> +  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
> +  # etc.) workflow runs are never canceled.
> +  #
> +  # We use a trick here: define the concurrency group as 'workflow
> +  # run ID' + # 'workflow run attempt' because it is a unique
> +  # combination for any run. So it effectively discards grouping.
> +  #
> +  # XXX: we cannot use `github.sha` as a unique identifier because
> +  # pushing a tag may cancel a run that works on a branch push
> +  # event.
> +  group: ${{ (
> +    github.ref == 'refs/heads/tarantool' ||
> +    startsWith(github.ref, 'refs/heads/tarantool-')) &&
> +    format('{0}-{1}', github.run_id, github.run_attempt) ||
> +    format('{0}-{1}', github.workflow, github.ref) }}
> +  cancel-in-progress: true
> +
> +jobs:
> +  test-x86_64-exotic:
> +    strategy:
> +      fail-fast: false
> +      matrix:
> +        BUILDTYPE: [Debug, Release]
> +        GC64: [ON, OFF]
> +        FLAVOR: [dualnum]
> +        include:
> +          - BUILDTYPE: Debug
> +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> +          - BUILDTYPE: Release
> +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo

Shall we add the -DLUAJIT_ENABLE_CHECKHOOK and -DLUAJIT_SMART_STRINGS=1 since they are in Tarantool by default?

> +          - FLAVOR: dualnum
> +            FLAVORFLAGS: -DLUAJIT_NUMMODE=2
> +    runs-on: [self-hosted, regular, Linux, x86_64]
> +    name: >
> +      LuaJIT ${{ matrix.FLAVOR }}
> +      (Linux/x86_64)
> +      ${{ matrix.BUILDTYPE }}
> +      GC64:${{ matrix.GC64 }}
> +    steps:
> +      - uses: actions/checkout@v2.3.4
> +        with:
> +          fetch-depth: 0
> +          submodules: recursive
> +      - name: setup Linux
> +        uses: ./.github/actions/setup-linux
> +      - name: configure
> +        run: >
> +          cmake -S . -B ${{ env.BUILDDIR }}
> +          -G Ninja
> +          ${{ matrix.CMAKEFLAGS }}
> +          ${{ matrix.FLAVORFLAGS }}
> +          -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
> +      - name: build
> +        run: cmake --build . --parallel
> +        working-directory: ${{ env.BUILDDIR }}
> +      - name: test
> +        run: cmake --build . --parallel --target test
> +        working-directory: ${{ env.BUILDDIR }}
> -- 
> 2.34.1
> 

Re: [Tarantool-patches] [PATCH luajit 2/3] ci: introduce workflow for exotic builds

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 27.01.23, sergos wrote:
> Hi!
> 
> Thanks for the patch!
> 
> Some question on build options.
> 
> Sergos
> 
> > On 18 Jan 2023, at 23:16, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > This workflow is well-suited for test specific builds like dual-number
> > mode on x86_x64, build with disabled JIT or FFI, etc.
> > 
> > For now, just the dual-number mode is added, due to the need to test
> > future commits. If there is need to add a new build type `FLAVOR` column
>                              ^                            ^---------
>                              a                           then(?) the
> 
> > of CI matrix should be exposed. Also, just Linux x86_64 is tested,
>                         extended?      xxxxx                      ^ only
> 

Thanks, fixed!
The new commit message is the following:

| ci: introduce workflow for exotic builds
|
| This workflow is well-suited for test specific builds like dual-number
| mode on x86_x64, build with disabled JIT or FFI, etc.
|
| For now, just the dual-number mode is added, due to the need to test
| future commits. If there is a need to add a new build type, then the
| `FLAVOR` column of CI matrix should be extended. Also, only Linux x86_64
| is tested, because we want to decrease resource usage in CI.

> > because we want to decrease resource usage in CI.
> > ---
> > .github/workflows/exotic-builds-testing.yml | 71 +++++++++++++++++++++
> > 1 file changed, 71 insertions(+)
> > create mode 100644 .github/workflows/exotic-builds-testing.yml
> > 
> > diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml
> > new file mode 100644
> > index 00000000..cd0c14d7
> > --- /dev/null
> > +++ b/.github/workflows/exotic-builds-testing.yml
> > @@ -0,0 +1,71 @@
> > +name: "Exotic builds testing"
> > +
> > +on:
> > +  push:
> > +    branches-ignore:
> > +      - '**-notest'
> > +      - 'upstream-**'
> > +    tags-ignore:
> > +      - '**'
> > +
> > +concurrency:
> > +  # An update of a developer branch cancels the previously
> > +  # scheduled workflow run for this branch. However, the default
> > +  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
> > +  # etc.) workflow runs are never canceled.
> > +  #
> > +  # We use a trick here: define the concurrency group as 'workflow
> > +  # run ID' + # 'workflow run attempt' because it is a unique
> > +  # combination for any run. So it effectively discards grouping.
> > +  #
> > +  # XXX: we cannot use `github.sha` as a unique identifier because
> > +  # pushing a tag may cancel a run that works on a branch push
> > +  # event.
> > +  group: ${{ (
> > +    github.ref == 'refs/heads/tarantool' ||
> > +    startsWith(github.ref, 'refs/heads/tarantool-')) &&
> > +    format('{0}-{1}', github.run_id, github.run_attempt) ||
> > +    format('{0}-{1}', github.workflow, github.ref) }}
> > +  cancel-in-progress: true
> > +
> > +jobs:
> > +  test-x86_64-exotic:
> > +    strategy:
> > +      fail-fast: false
> > +      matrix:
> > +        BUILDTYPE: [Debug, Release]
> > +        GC64: [ON, OFF]
> > +        FLAVOR: [dualnum]
> > +        include:
> > +          - BUILDTYPE: Debug
> > +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> > +          - BUILDTYPE: Release
> > +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
> 
> Shall we add the -DLUAJIT_ENABLE_CHECKHOOK and -DLUAJIT_SMART_STRINGS=1 since they are in Tarantool by default?

-DLUAJIT_SMART_STRINGS=1 is set by default, so it's OK not to set it
mannually.

OTOH, -DLUAJIT_ENABLE_CHECKHOOK is not set by default.
I prefer not to set it for this particular workflow, because I plan to
use it for very specific builds (even impossible for Tarantool) for
example with -DLUAJIT_DISABLE_JIT=ON, -DLUAJIT_DISABLE_FFI=ON flags.
Also, this flag is not set for default LuaJIT testing, so I tried to
make those workflows look similar.
But if we want to add this flag for default build or testing we should
add it for this workflow too.
CC-ed Igor to hear his opinion.

> 
> > +          - FLAVOR: dualnum
> > +            FLAVORFLAGS: -DLUAJIT_NUMMODE=2
> > +    runs-on: [self-hosted, regular, Linux, x86_64]
> > +    name: >
> > +      LuaJIT ${{ matrix.FLAVOR }}
> > +      (Linux/x86_64)
> > +      ${{ matrix.BUILDTYPE }}
> > +      GC64:${{ matrix.GC64 }}
> > +    steps:
> > +      - uses: actions/checkout@v2.3.4
> > +        with:
> > +          fetch-depth: 0
> > +          submodules: recursive
> > +      - name: setup Linux
> > +        uses: ./.github/actions/setup-linux
> > +      - name: configure
> > +        run: >
> > +          cmake -S . -B ${{ env.BUILDDIR }}
> > +          -G Ninja
> > +          ${{ matrix.CMAKEFLAGS }}
> > +          ${{ matrix.FLAVORFLAGS }}
> > +          -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
> > +      - name: build
> > +        run: cmake --build . --parallel
> > +        working-directory: ${{ env.BUILDDIR }}
> > +      - name: test
> > +        run: cmake --build . --parallel --target test
> > +        working-directory: ${{ env.BUILDDIR }}
> > -- 
> > 2.34.1
> > 
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/3] ci: introduce workflow for exotic builds

[Igor Munkin via Tarantool-patches]

Hi everyone,

On 30.01.23, Sergey Kaplun wrote:

<snipped>

> > 
> > Shall we add the -DLUAJIT_ENABLE_CHECKHOOK and -DLUAJIT_SMART_STRINGS=1 since they are in Tarantool by default?
> 
> -DLUAJIT_SMART_STRINGS=1 is set by default, so it's OK not to set it
> mannually.
> 
> OTOH, -DLUAJIT_ENABLE_CHECKHOOK is not set by default.
> I prefer not to set it for this particular workflow, because I plan to
> use it for very specific builds (even impossible for Tarantool) for
> example with -DLUAJIT_DISABLE_JIT=ON, -DLUAJIT_DISABLE_FFI=ON flags.
> Also, this flag is not set for default LuaJIT testing, so I tried to
> make those workflows look similar.
> But if we want to add this flag for default build or testing we should
> add it for this workflow too.
> CC-ed Igor to hear his opinion.

I'm totally for adding such workflows, but I suggest to do it in a
separate patch. Sergey, this is on you.

> 
> > 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/3] ci: introduce workflow for exotic builds

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!

Thanks for reply:)!

On 01.02.23, Igor Munkin wrote:
> Hi everyone,
> 
> On 30.01.23, Sergey Kaplun wrote:
> 
> <snipped>
> 
> > > 
> > > Shall we add the -DLUAJIT_ENABLE_CHECKHOOK and -DLUAJIT_SMART_STRINGS=1 since they are in Tarantool by default?
> > 
> > -DLUAJIT_SMART_STRINGS=1 is set by default, so it's OK not to set it
> > mannually.
> > 
> > OTOH, -DLUAJIT_ENABLE_CHECKHOOK is not set by default.
> > I prefer not to set it for this particular workflow, because I plan to
> > use it for very specific builds (even impossible for Tarantool) for
> > example with -DLUAJIT_DISABLE_JIT=ON, -DLUAJIT_DISABLE_FFI=ON flags.
> > Also, this flag is not set for default LuaJIT testing, so I tried to
> > make those workflows look similar.
> > But if we want to add this flag for default build or testing we should
> > add it for this workflow too.
> > CC-ed Igor to hear his opinion.
> 
> I'm totally for adding such workflows, but I suggest to do it in a
> separate patch. Sergey, this is on you.

OK, so I'll add exotic -DLUAJIT_ENBALE_CHECKHOOK job after this patch
will be merged to avoid rebases.

> 
> > 
> > > 
> 
> <snipped>
> 
> > 
> > -- 
> > Best regards,
> > Sergey Kaplun
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/3] ci: introduce workflow for exotic builds

[sergos via Tarantool-patches]

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

Hi!

Thanks for the update!
That’s exactly was on my mind - an additional job, not changing this one.
LGTM.

Sergos

> On 1 Feb 2023, at 11:52, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Igor!
> 
> Thanks for reply:)!
> 
> On 01.02.23, Igor Munkin wrote:
>> Hi everyone,
>> 
>> On 30.01.23, Sergey Kaplun wrote:
>> 
>> <snipped>
>> 
>>>> 
>>>> Shall we add the -DLUAJIT_ENABLE_CHECKHOOK and -DLUAJIT_SMART_STRINGS=1 since they are in Tarantool by default?
>>> 
>>> -DLUAJIT_SMART_STRINGS=1 is set by default, so it's OK not to set it
>>> mannually.
>>> 
>>> OTOH, -DLUAJIT_ENABLE_CHECKHOOK is not set by default.
>>> I prefer not to set it for this particular workflow, because I plan to
>>> use it for very specific builds (even impossible for Tarantool) for
>>> example with -DLUAJIT_DISABLE_JIT=ON, -DLUAJIT_DISABLE_FFI=ON flags.
>>> Also, this flag is not set for default LuaJIT testing, so I tried to
>>> make those workflows look similar.
>>> But if we want to add this flag for default build or testing we should
>>> add it for this workflow too.
>>> CC-ed Igor to hear his opinion.
>> 
>> I'm totally for adding such workflows, but I suggest to do it in a
>> separate patch. Sergey, this is on you.
> 
> OK, so I'll add exotic -DLUAJIT_ENBALE_CHECKHOOK job after this patch
> will be merged to avoid rebases.
> 
>> 
>>> 
>>>> 
>> 
>> <snipped>
>> 
>>> 
>>> -- 
>>> Best regards,
>>> Sergey Kaplun
>> 
>> -- 
>> Best regards,
>> IM
> 
> -- 
> Best regards,
> Sergey Kaplun


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

[Tarantool-patches] [PATCH luajit 3/3] x86/x64: Fix loop realignment.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

(cherry picked from commit 522d2073da4be2af79db4728cbb375db0fbdfc48)

`asm_intarith()` function may try to drop `test r, r` instruction before
the Jcc instruction. However, in case when Jcc instruction is "Jump
short if ..." instruction (i.e. has no 0F opcode prefix like "Jump near
if ..."), the `test` instruction is dropped when shouldn't be, due to
memory miss. As the result, the loop can't be realigned later in
`asm_loop_fixup` due to target to jump isn't aligned and the assertion
fails.

This patch adds the additional check for 0F opcode in `asm_intarith()`.

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

Part of tarantool/tarantool#8069
---
 src/lj_asm_x86.h                               |  5 +++--
 .../lj-556-fix-loop-realignment.test.lua       | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)
 create mode 100644 test/tarantool-tests/lj-556-fix-loop-realignment.test.lua

diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
index 8efda8e5..e6c42c6d 100644
--- a/src/lj_asm_x86.h
+++ b/src/lj_asm_x86.h
@@ -2068,8 +2068,9 @@ static void asm_intarith(ASMState *as, IRIns *ir, x86Arith xa)
   int32_t k = 0;
   if (as->flagmcp == as->mcp) {  /* Drop test r,r instruction. */
     MCode *p = as->mcp + ((LJ_64 && *as->mcp < XI_TESTb) ? 3 : 2);
-    if ((p[1] & 15) < 14) {
-      if ((p[1] & 15) >= 12) p[1] -= 4;  /* L <->S, NL <-> NS */
+    MCode *q = p[0] == 0x0f ? p+1 : p;
+    if ((*q & 15) < 14) {
+      if ((*q & 15) >= 12) *q -= 4;  /* L <->S, NL <-> NS */
       as->flagmcp = NULL;
       as->mcp = p;
     }  /* else: cannot transform LE/NLE to cc without use of OF. */
diff --git a/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
new file mode 100644
index 00000000..9a8e6098
--- /dev/null
+++ b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
@@ -0,0 +1,18 @@
+local tap = require('tap')
+
+local test = tap.test('lj-505-fold-icorrect-behavior')
+test:plan(1)
+
+-- Test file to demonstrate JIT misbehaviour for loop realignment
+-- in LUAJIT_NUMMODE=2. See also
+-- https://github.com/LuaJIT/LuaJIT/issues/556.
+
+jit.opt.start('hotloop=1')
+
+local s = 4
+while s > 0 do
+  s = s - 1
+end
+
+test:ok(true, 'loop is compiled and ran successfully')
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 3/3] x86/x64: Fix loop realignment.

[Sergey Kaplun via Tarantool-patches]

<snipped>
> +++ b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
> @@ -0,0 +1,18 @@
> +local tap = require('tap')
> +
> +local test = tap.test('lj-505-fold-icorrect-behavior')

Fixed this typo, branch is force-pushed.

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 3/3] x86/x64: Fix loop realignment.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
 
> 
>>From: Mike Pall <mike>
>>
>>(cherry picked from commit 522d2073da4be2af79db4728cbb375db0fbdfc48)
>>
>>`asm_intarith()` function may try to drop `test r, r` instruction before
>Please note that "r" is an allocated register for the instruction.
>>the Jcc instruction. However, in case when Jcc instruction is "Jump
>Typo: s/in case when/in cases where/
>>short if ..." instruction (i.e. has no 0F opcode prefix like "Jump near
>>if ..."), the `test` instruction is dropped when shouldn't be, due to
>Typo: s/when/when it/
>>memory miss. As the result, the loop can't be realigned later in
>Typo: s/memory/a memory/
>Also, that part about the memory miss is unclear, it would be better if you
>could clarify it a bit.
>>`asm_loop_fixup` due to target to jump isn't aligned and the assertion
>Typo: s/isn’t aligned/being misaligned/
>>fails.
>>
>>This patch adds the additional check for 0F opcode in `asm_intarith()`.
>Typo: s/for 0F/for the 0F/
>>
>>Sergey Kaplun:
>>* added the description and the test for the problem
>>
>>Part of tarantool/tarantool#8069
>>---
>> src/lj_asm_x86.h | 5 +++--
>> .../lj-556-fix-loop-realignment.test.lua | 18 ++++++++++++++++++
>> 2 files changed, 21 insertions(+), 2 deletions(-)
>> create mode 100644 test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
>>
>>diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
>>index 8efda8e5..e6c42c6d 100644
>>--- a/src/lj_asm_x86.h
>>+++ b/src/lj_asm_x86.h
>>@@ -2068,8 +2068,9 @@ static void asm_intarith(ASMState *as, IRIns *ir, x86Arith xa)
>>   int32_t k = 0;
>>   if (as->flagmcp == as->mcp) { /* Drop test r,r instruction. */
>>     MCode *p = as->mcp + ((LJ_64 && *as->mcp < XI_TESTb) ? 3 : 2);
>>- if ((p[1] & 15) < 14) {
>>- if ((p[1] & 15) >= 12) p[1] -= 4; /* L <->S, NL <-> NS */
>>+ MCode *q = p[0] == 0x0f ? p+1 : p;
>>+ if ((*q & 15) < 14) {
>>+ if ((*q & 15) >= 12) *q -= 4; /* L <->S, NL <-> NS */
>>       as->flagmcp = NULL;
>>       as->mcp = p;
>>     } /* else: cannot transform LE/NLE to cc without use of OF. */
>>diff --git a/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
>>new file mode 100644
>>index 00000000..9a8e6098
>>--- /dev/null
>>+++ b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
>>@@ -0,0 +1,18 @@
>>+local tap = require('tap')
>>+
>>+local test = tap.test('lj-505-fold-icorrect-behavior')
>>+test:plan(1)
>>+
>>+-- Test file to demonstrate JIT misbehaviour for loop realignment
>>+-- in LUAJIT_NUMMODE=2. See also
>>+--  https://github.com/LuaJIT/LuaJIT/issues/556 .
>>+
>>+jit.opt.start('hotloop=1')
>>+
>>+local s = 4
>>+while s > 0 do
>>+ s = s - 1
>>+end
>>+
>>+test:ok(true, 'loop is compiled and ran successfully')
>>+os.exit(test:check() and 0 or 1)
>>--
>The test works just fine with HEAD on 
>f7d61d96  ci: introduce workflow for exotic builds.
> 
>Tested configurations: 
>LJ_64: True, LJ_GC64: True, LJ_DUALNUM: True
>LJ_64: True, LJ_GC64: False, LJ_DUALNUM: True
>--
>Best regards,
>Maxim Kokryashkin
> 

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

Re: [Tarantool-patches] [PATCH luajit 3/3] x86/x64: Fix loop realignment.

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!

Thanks for the review!

On 24.01.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>  
> > 
> >>From: Mike Pall <mike>
> >>
> >>(cherry picked from commit 522d2073da4be2af79db4728cbb375db0fbdfc48)
> >>
> >>`asm_intarith()` function may try to drop `test r, r` instruction before
> >Please note that "r" is an allocated register for the instruction.
> >>the Jcc instruction. However, in case when Jcc instruction is "Jump
> >Typo: s/in case when/in cases where/

Fixed.

> >>short if ..." instruction (i.e. has no 0F opcode prefix like "Jump near
> >>if ..."), the `test` instruction is dropped when shouldn't be, due to
> >Typo: s/when/when it/
> >>memory miss. As the result, the loop can't be realigned later in
> >Typo: s/memory/a memory/
> >Also, that part about the memory miss is unclear, it would be better if you
> >could clarify it a bit.
> >>`asm_loop_fixup` due to target to jump isn't aligned and the assertion
> >Typo: s/isn’t aligned/being misaligned/

Fixed.

> >>fails.
> >>
> >>This patch adds the additional check for 0F opcode in `asm_intarith()`.
> >Typo: s/for 0F/for the 0F/

Fixed, thanks!

> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8069

The new commit message is the following:

| x86/x64: Fix loop realignment.
|
| (cherry picked from commit 522d2073da4be2af79db4728cbb375db0fbdfc48)
|
| `asm_intarith()` function may try to drop `test r, r` (where `r` is an
| allocated register) instruction before the Jcc instruction. However, in
| cases when Jcc instruction is "Jump short if ..." instruction (i.e. has
| no 0F opcode prefix like "Jump near if ..."), the `test` instruction is
| dropped when it shouldn't be, due to usage for the comparison the next
| byte after instruction itself. As the result, the loop can't be
| realigned later in `asm_loop_fixup` due to target to jump being
| misaligned and the assertion fails.
|
| This patch adds the additional check for the 0F opcode in
| `asm_intarith()`.
|
| Sergey Kaplun:
| * added the description and the test for the problem
|
| Part of tarantool/tarantool#8069

Branch is force pushed.

> >>---
> >> src/lj_asm_x86.h | 5 +++--
> >> .../lj-556-fix-loop-realignment.test.lua | 18 ++++++++++++++++++
> >> 2 files changed, 21 insertions(+), 2 deletions(-)
> >> create mode 100644 test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
> >>
> >>diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> >>index 8efda8e5..e6c42c6d 100644
> >>--- a/src/lj_asm_x86.h
> >>+++ b/src/lj_asm_x86.h
> >>@@ -2068,8 +2068,9 @@ static void asm_intarith(ASMState *as, IRIns *ir, x86Arith xa)
> >>   int32_t k = 0;
> >>   if (as->flagmcp == as->mcp) { /* Drop test r,r instruction. */
> >>     MCode *p = as->mcp + ((LJ_64 && *as->mcp < XI_TESTb) ? 3 : 2);
> >>- if ((p[1] & 15) < 14) {
> >>- if ((p[1] & 15) >= 12) p[1] -= 4; /* L <->S, NL <-> NS */
> >>+ MCode *q = p[0] == 0x0f ? p+1 : p;
> >>+ if ((*q & 15) < 14) {
> >>+ if ((*q & 15) >= 12) *q -= 4; /* L <->S, NL <-> NS */
> >>       as->flagmcp = NULL;
> >>       as->mcp = p;
> >>     } /* else: cannot transform LE/NLE to cc without use of OF. */
> >>diff --git a/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
> >>new file mode 100644
> >>index 00000000..9a8e6098
> >>--- /dev/null
> >>+++ b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
> >>@@ -0,0 +1,18 @@
> >>+local tap = require('tap')
> >>+
> >>+local test = tap.test('lj-505-fold-icorrect-behavior')
> >>+test:plan(1)
> >>+
> >>+-- Test file to demonstrate JIT misbehaviour for loop realignment
> >>+-- in LUAJIT_NUMMODE=2. See also
> >>+--  https://github.com/LuaJIT/LuaJIT/issues/556 .
> >>+
> >>+jit.opt.start('hotloop=1')
> >>+
> >>+local s = 4
> >>+while s > 0 do
> >>+ s = s - 1
> >>+end
> >>+
> >>+test:ok(true, 'loop is compiled and ran successfully')
> >>+os.exit(test:check() and 0 or 1)
> >>--
> >The test works just fine with HEAD on 
> >f7d61d96  ci: introduce workflow for exotic builds.
> > 
> >Tested configurations: 
> >LJ_64: True, LJ_GC64: True, LJ_DUALNUM: True
> >LJ_64: True, LJ_GC64: False, LJ_DUALNUM: True

It's strange...
I use the following build command:
| $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DLUAJIT_ENABLE_GC64=OFF -DLUAJIT_NUMMODE=2 && make -j
and get the following assertion:
| asm_loop_fixup: Assertion `((intptr_t)target & 15) == 0' failed.
What command do you use to build LuaJIT?

> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 3/3] x86/x64: Fix loop realignment.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the fixes!
LGTM
 
> 
>>Hi, Maxim!
>>
>>Thanks for the review!
>>
>>On 24.01.23, Maxim Kokryashkin wrote:
>>>
>>> Hi, Sergey!
>>> Thanks for the patch!
>>> Please consider my comments below.
>>>  
>>> > 
>>> >>From: Mike Pall <mike>
>>> >>
>>> >>(cherry picked from commit 522d2073da4be2af79db4728cbb375db0fbdfc48)
>>> >>
>>> >>`asm_intarith()` function may try to drop `test r, r` instruction before
>>> >Please note that "r" is an allocated register for the instruction.
>>> >>the Jcc instruction. However, in case when Jcc instruction is "Jump
>>> >Typo: s/in case when/in cases where/
>>
>>Fixed.
>>
>>> >>short if ..." instruction (i.e. has no 0F opcode prefix like "Jump near
>>> >>if ..."), the `test` instruction is dropped when shouldn't be, due to
>>> >Typo: s/when/when it/
>>> >>memory miss. As the result, the loop can't be realigned later in
>>> >Typo: s/memory/a memory/
>>> >Also, that part about the memory miss is unclear, it would be better if you
>>> >could clarify it a bit.
>>> >>`asm_loop_fixup` due to target to jump isn't aligned and the assertion
>>> >Typo: s/isn’t aligned/being misaligned/
>>
>>Fixed.
>>
>>> >>fails.
>>> >>
>>> >>This patch adds the additional check for 0F opcode in `asm_intarith()`.
>>> >Typo: s/for 0F/for the 0F/
>>
>>Fixed, thanks!
>>
>>> >>
>>> >>Sergey Kaplun:
>>> >>* added the description and the test for the problem
>>> >>
>>> >>Part of tarantool/tarantool#8069
>>
>>The new commit message is the following:
>>
>>| x86/x64: Fix loop realignment.
>>|
>>| (cherry picked from commit 522d2073da4be2af79db4728cbb375db0fbdfc48)
>>|
>>| `asm_intarith()` function may try to drop `test r, r` (where `r` is an
>>| allocated register) instruction before the Jcc instruction. However, in
>>| cases when Jcc instruction is "Jump short if ..." instruction (i.e. has
>>| no 0F opcode prefix like "Jump near if ..."), the `test` instruction is
>>| dropped when it shouldn't be, due to usage for the comparison the next
>>| byte after instruction itself. As the result, the loop can't be
>>| realigned later in `asm_loop_fixup` due to target to jump being
>>| misaligned and the assertion fails.
>>|
>>| This patch adds the additional check for the 0F opcode in
>>| `asm_intarith()`.
>>|
>>| Sergey Kaplun:
>>| * added the description and the test for the problem
>>|
>>| Part of tarantool/tarantool#8069
>>
>>Branch is force pushed.
>>
>>> >>---
>>> >> src/lj_asm_x86.h | 5 +++--
>>> >> .../lj-556-fix-loop-realignment.test.lua | 18 ++++++++++++++++++
>>> >> 2 files changed, 21 insertions(+), 2 deletions(-)
>>> >> create mode 100644 test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
>>> >>
>>> >>diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
>>> >>index 8efda8e5..e6c42c6d 100644
>>> >>--- a/src/lj_asm_x86.h
>>> >>+++ b/src/lj_asm_x86.h
>>> >>@@ -2068,8 +2068,9 @@ static void asm_intarith(ASMState *as, IRIns *ir, x86Arith xa)
>>> >>   int32_t k = 0;
>>> >>   if (as->flagmcp == as->mcp) { /* Drop test r,r instruction. */
>>> >>     MCode *p = as->mcp + ((LJ_64 && *as->mcp < XI_TESTb) ? 3 : 2);
>>> >>- if ((p[1] & 15) < 14) {
>>> >>- if ((p[1] & 15) >= 12) p[1] -= 4; /* L <->S, NL <-> NS */
>>> >>+ MCode *q = p[0] == 0x0f ? p+1 : p;
>>> >>+ if ((*q & 15) < 14) {
>>> >>+ if ((*q & 15) >= 12) *q -= 4; /* L <->S, NL <-> NS */
>>> >>       as->flagmcp = NULL;
>>> >>       as->mcp = p;
>>> >>     } /* else: cannot transform LE/NLE to cc without use of OF. */
>>> >>diff --git a/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
>>> >>new file mode 100644
>>> >>index 00000000..9a8e6098
>>> >>--- /dev/null
>>> >>+++ b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
>>> >>@@ -0,0 +1,18 @@
>>> >>+local tap = require('tap')
>>> >>+
>>> >>+local test = tap.test('lj-505-fold-icorrect-behavior')
>>> >>+test:plan(1)
>>> >>+
>>> >>+-- Test file to demonstrate JIT misbehaviour for loop realignment
>>> >>+-- in LUAJIT_NUMMODE=2. See also
>>> >>+--  https://github.com/LuaJIT/LuaJIT/issues/556 .
>>> >>+
>>> >>+jit.opt.start('hotloop=1')
>>> >>+
>>> >>+local s = 4
>>> >>+while s > 0 do
>>> >>+ s = s - 1
>>> >>+end
>>> >>+
>>> >>+test:ok(true, 'loop is compiled and ran successfully')
>>> >>+os.exit(test:check() and 0 or 1)
>>> >>--
>>> >The test works just fine with HEAD on 
>>> >f7d61d96  ci: introduce workflow for exotic builds.
>>> > 
>>> >Tested configurations: 
>>> >LJ_64: True, LJ_GC64: True, LJ_DUALNUM: True
>>> >LJ_64: True, LJ_GC64: False, LJ_DUALNUM: True
>>
>>It's strange...
>>I use the following build command:
>>| $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DLUAJIT_ENABLE_GC64=OFF -DLUAJIT_NUMMODE=2 && make -j
>>and get the following assertion:
>>| asm_loop_fixup: Assertion `((intptr_t)target & 15) == 0' failed.
>>What command do you use to build LuaJIT?
>PEBKAC, I forgot to add the LUA_USE_ASSERT flag.
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
>--
>Best regards,
>Maxim Kokryashkin
> 

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

Re: [Tarantool-patches] [PATCH luajit 3/3] x86/x64: Fix loop realignment.

[sergos via Tarantool-patches]

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

Hi!

Thanks for the patch, it is LGTM.

Sergos


> 
> The new commit message is the following:
> 
> | x86/x64: Fix loop realignment.
> |
> | (cherry picked from commit 522d2073da4be2af79db4728cbb375db0fbdfc48)
> |
> | `asm_intarith()` function may try to drop `test r, r` (where `r` is an
> | allocated register) instruction before the Jcc instruction. However, in
> | cases when Jcc instruction is "Jump short if ..." instruction (i.e. has
> | no 0F opcode prefix like "Jump near if ..."), the `test` instruction is
> | dropped when it shouldn't be, due to usage for the comparison the next
> | byte after instruction itself. As the result, the loop can't be
> | realigned later in `asm_loop_fixup` due to target to jump being
> | misaligned and the assertion fails.
> |
> | This patch adds the additional check for the 0F opcode in
> | `asm_intarith()`.
> |
> | Sergey Kaplun:
> | * added the description and the test for the problem
> |
> | Part of tarantool/tarantool#8069
> 
> Branch is force pushed.
> 
>>>> ---
>>>>  src/lj_asm_x86.h | 5 +++--
>>>>  .../lj-556-fix-loop-realignment.test.lua | 18 ++++++++++++++++++
>>>>  2 files changed, 21 insertions(+), 2 deletions(-)
>>>>  create mode 100644 test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
>>>> 
>>>> diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
>>>> index 8efda8e5..e6c42c6d 100644
>>>> --- a/src/lj_asm_x86.h
>>>> +++ b/src/lj_asm_x86.h
>>>> @@ -2068,8 +2068,9 @@ static void asm_intarith(ASMState *as, IRIns *ir, x86Arith xa)
>>>>    int32_t k = 0;
>>>>    if (as->flagmcp == as->mcp) { /* Drop test r,r instruction. */
>>>>      MCode *p = as->mcp + ((LJ_64 && *as->mcp < XI_TESTb) ? 3 : 2);
>>>> - if ((p[1] & 15) < 14) {
>>>> - if ((p[1] & 15) >= 12) p[1] -= 4; /* L <->S, NL <-> NS */
>>>> + MCode *q = p[0] == 0x0f ? p+1 : p;
>>>> + if ((*q & 15) < 14) {
>>>> + if ((*q & 15) >= 12) *q -= 4; /* L <->S, NL <-> NS */
>>>>        as->flagmcp = NULL;
>>>>        as->mcp = p;
>>>>      } /* else: cannot transform LE/NLE to cc without use of OF. */
>>>> diff --git a/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
>>>> new file mode 100644
>>>> index 00000000..9a8e6098
>>>> --- /dev/null
>>>> +++ b/test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
>>>> @@ -0,0 +1,18 @@
>>>> +local tap = require('tap')
>>>> +
>>>> +local test = tap.test('lj-505-fold-icorrect-behavior')
>>>> +test:plan(1)
>>>> +
>>>> +-- Test file to demonstrate JIT misbehaviour for loop realignment
>>>> +-- in LUAJIT_NUMMODE=2. See also
>>>> +--  https://github.com/LuaJIT/LuaJIT/issues/556 .
>>>> +
>>>> +jit.opt.start('hotloop=1')
>>>> +
>>>> +local s = 4
>>>> +while s > 0 do
>>>> + s = s - 1
>>>> +end
>>>> +
>>>> +test:ok(true, 'loop is compiled and ran successfully')
>>>> +os.exit(test:check() and 0 or 1)
>>>> --
>>> The test works just fine with HEAD on 
>>> f7d61d96  ci: introduce workflow for exotic builds.
>>>  
>>> Tested configurations: 
>>> LJ_64: True, LJ_GC64: True, LJ_DUALNUM: True
>>> LJ_64: True, LJ_GC64: False, LJ_DUALNUM: True
> 
> It's strange...
> I use the following build command:
> | $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DLUAJIT_ENABLE_GC64=OFF -DLUAJIT_NUMMODE=2 && make -j
> and get the following assertion:
> | asm_loop_fixup: Assertion `((intptr_t)target & 15) == 0' failed.
> What command do you use to build LuaJIT?
> 
>>> --
>>> Best regards,
>>> Maxim Kokryashkin
>>>  
> 
> -- 
> Best regards,
> Sergey Kaplun


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

Re: [Tarantool-patches] [PATCH luajit 0/3] Dualnumber mode fixes

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patches into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

On 18.01.23, Sergey Kaplun via Tarantool-patches wrote:
> Something strange happened with the previous cover-letter, so resend
> it.
> 
> The first 2 patches:
> * sysprof: fix interval parsing in dual-number mode
> * ci: introduce workflow for exotic builds
> are needed to introduce workflow for dualnumber mode build, to test the
> 3rd commit:
> * x86/x64: Fix loop realignment.
> 
> The first and second commits are separated as far as only the first one
> is needed to backport to tarantool-1.10 branch.
> 
> PR: https://github.com/tarantool/tarantool/pull/8180
> Issues:
> * https://github.com/LuaJIT/LuaJIT/issues/556
> * https://github.com/tarantool/tarantool/issues/8069
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-556-fix-loop-realignment-full-ci
> 
> Mike Pall (1):
>   x86/x64: Fix loop realignment.
> 
> Sergey Kaplun (2):
>   sysprof: fix interval parsing in dual-number mode
>   ci: introduce workflow for exotic builds
> 
>  .github/workflows/exotic-builds-testing.yml   | 71 +++++++++++++++++++
>  src/lib_misc.c                                |  2 +-
>  src/lj_asm_x86.h                              |  5 +-
>  .../lj-556-fix-loop-realignment.test.lua      | 18 +++++
>  4 files changed, 93 insertions(+), 3 deletions(-)
>  create mode 100644 .github/workflows/exotic-builds-testing.yml
>  create mode 100644 test/tarantool-tests/lj-556-fix-loop-realignment.test.lua
> 
> -- 
> 2.34.1
> 

-- 
Best regards,
IM