[Tarantool-patches] [PATCH luajit 0/2] Add ASan support in LuaJIT

[Tarantool-patches] [PATCH luajit 0/2] Add ASan support in LuaJIT

[Igor Munkin via Tarantool-patches]

This series enables ASan support back for LuaJIT repository. The only
thing that was missing the whole time was the option per se (and the
corresponding compile flags, obviously). Anyway, when the option has
been finally added, the dedicated CI workflow has been added too in
scope of the second patch in the series. All other info can be found in
the corresponding patches.

Issue: https://github.com/tarantool/tarantool/issues/5878
Branch: https://github.com/tarantool/luajit/tree/hackaton/gh-5878-enable-ASAN
Tarantool related changes and CI can be found in #8846[1].

Igor Munkin (2):
  build: introduce LUAJIT_USE_ASAN option
  ci: introduce testing workflow with sanitizers

 .github/actions/setup-sanitizers/README.md  | 12 ++++
 .github/actions/setup-sanitizers/action.yml | 24 +++++++
 .github/workflows/sanitizers-testing.yml    | 79 +++++++++++++++++++++
 CMakeLists.txt                              | 16 ++---
 4 files changed, 122 insertions(+), 9 deletions(-)
 create mode 100644 .github/actions/setup-sanitizers/README.md
 create mode 100644 .github/actions/setup-sanitizers/action.yml
 create mode 100644 .github/workflows/sanitizers-testing.yml

[1]: https://github.com/tarantool/tarantool/pull/8846

-- 
2.30.2

[Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_ASAN option

[Igor Munkin via Tarantool-patches]

There was nether a special option nor a variable to configure the
sanitizers support via the build system since the commit
052c1a32a5e74b337686eee229d089d576775931 ("Add preliminary ASAN
support") where ASan support had been introduced. We finally decided to
use this feature in LuaJIT CI, so for convenient build configuration
LUAJIT_USE_ASAN option is added to the root CMakeLists.txt.

Resolves tarantool/tarantool#5878

Co-authored-by: Sergey Kaplun <skaplun@tarantool.org>
Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 CMakeLists.txt | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 6ef24bba..f113a53d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -269,15 +269,13 @@ if(LUA_USE_ASSERT)
   AppendFlags(TARGET_C_FLAGS -DLUA_USE_ASSERT)
 endif()
 
-# TODO: Implement a configuration option to enable ASAN.
-# There are two entries of LUAJIT_USE_ASAN define:
-# $ grep -rnF 'LUAJIT_USE_ASAN' .
-# ./src/lj_str.c:15:#if LUAJIT_USE_ASAN
-# ./src/host/buildvm.c:36:#if LUAJIT_USE_ASAN
-# At the same time this flag is not provided by LuaJIT original
-# build system (i.e. src/Makefile.original) so there are no
-# related compiler and linker flags passed. This should be done
-# the right way later.
+# Turn on AddressSanitizer support. As a result, all artefacts
+# (i.e. buildvm, LuaJIT, testing infrastructure) are built with
+# ASan enabled.
+option(LUAJIT_USE_ASAN "Build LuaJIT with AddressSanitizer" OFF)
+if(LUAJIT_USE_ASAN)
+  AppendFlags(CMAKE_C_FLAGS -DLUAJIT_USE_ASAN -fsanitize=address)
+endif()
 
 # --- Main source tree ---------------------------------------------------------
 
-- 
2.30.2

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_ASAN option

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_ASAN option

[Sergey Bronnikov via Tarantool-patches]

Hi, Igor!

Thanks for the patch! This patch LGTM after fixing a typo.

Sergey

On 7/13/23 00:09, Igor Munkin wrote:
> There was nether a special option nor a variable to configure the
Typo: nether -> neither
> sanitizers support via the build system since the commit
> 052c1a32a5e74b337686eee229d089d576775931 ("Add preliminary ASAN
> support") where ASan support had been introduced. We finally decided to
> use this feature in LuaJIT CI, so for convenient build configuration
> LUAJIT_USE_ASAN option is added to the root CMakeLists.txt.
>
> Resolves tarantool/tarantool#5878
>
> Co-authored-by: Sergey Kaplun <skaplun@tarantool.org>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>   CMakeLists.txt | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 6ef24bba..f113a53d 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -269,15 +269,13 @@ if(LUA_USE_ASSERT)
>     AppendFlags(TARGET_C_FLAGS -DLUA_USE_ASSERT)
>   endif()
>   
> -# TODO: Implement a configuration option to enable ASAN.
> -# There are two entries of LUAJIT_USE_ASAN define:
> -# $ grep -rnF 'LUAJIT_USE_ASAN' .
> -# ./src/lj_str.c:15:#if LUAJIT_USE_ASAN
> -# ./src/host/buildvm.c:36:#if LUAJIT_USE_ASAN
> -# At the same time this flag is not provided by LuaJIT original
> -# build system (i.e. src/Makefile.original) so there are no
> -# related compiler and linker flags passed. This should be done
> -# the right way later.
> +# Turn on AddressSanitizer support. As a result, all artefacts
> +# (i.e. buildvm, LuaJIT, testing infrastructure) are built with
> +# ASan enabled.
> +option(LUAJIT_USE_ASAN "Build LuaJIT with AddressSanitizer" OFF)
> +if(LUAJIT_USE_ASAN)
> +  AppendFlags(CMAKE_C_FLAGS -DLUAJIT_USE_ASAN -fsanitize=address)
> +endif()
>   
>   # --- Main source tree ---------------------------------------------------------
>   

Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_ASAN option

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review! I've fixed the typo you're mentioning. See the
second version of the series[1] for updates.

[1]: https://lists.tarantool.org/tarantool-patches/cover.1689925402.git.imun@tarantool.org/T/#t

-- 
Best regards,
IM

[Tarantool-patches] [PATCH luajit 2/2] ci: introduce testing workflow with sanitizers

[Igor Munkin via Tarantool-patches]

This commit adds GitHub workflow running all available LuaJIT tests with
LUAJIT_USE_ASAN option enabled. For now, sanitizers workflow works only
for Linux/x86_64 as the most scaling setup in our CI. I believe we will
be able to add other platforms being supported, when ARM64 and macOS
runners starvation is defeated. There is also a separate GitHub Action
introduced for convenient setup of the environment.

Follows up tarantool/tarantool#5878

Signed-off-by: Igor Munkin <imun@tarantool.org>
---
 .github/actions/setup-sanitizers/README.md  | 12 ++++
 .github/actions/setup-sanitizers/action.yml | 24 +++++++
 .github/workflows/sanitizers-testing.yml    | 79 +++++++++++++++++++++
 3 files changed, 115 insertions(+)
 create mode 100644 .github/actions/setup-sanitizers/README.md
 create mode 100644 .github/actions/setup-sanitizers/action.yml
 create mode 100644 .github/workflows/sanitizers-testing.yml

diff --git a/.github/actions/setup-sanitizers/README.md b/.github/actions/setup-sanitizers/README.md
new file mode 100644
index 00000000..3aa9e214
--- /dev/null
+++ b/.github/actions/setup-sanitizers/README.md
@@ -0,0 +1,12 @@
+# Setup environment for sanitizers on Linux
+
+Action setups the environment on Linux runners (install requirements, setup the
+workflow environment, etc) for testing with sanitizers enabled.
+
+## How to use Github Action from Github workflow
+
+Add the following code to the running steps before LuaJIT configuration:
+```
+- uses: ./.github/actions/setup-sanitizers
+  if: ${{ matrix.OS == 'Linux' }}
+```
diff --git a/.github/actions/setup-sanitizers/action.yml b/.github/actions/setup-sanitizers/action.yml
new file mode 100644
index 00000000..ca6b6b9f
--- /dev/null
+++ b/.github/actions/setup-sanitizers/action.yml
@@ -0,0 +1,24 @@
+name: Setup CI environment for testing with sanitizers on Linux
+description: Common part to tweak Linux CI runner environment for sanitizers
+runs:
+  using: composite
+  steps:
+    - name: Setup CI environment
+      uses: ./.github/actions/setup
+    - name: Set CMAKE_BUILD_PARALLEL_LEVEL
+      run: |
+        # Set CMAKE_BUILD_PARALLEL_LEVEL environment variable to
+        # limit the number of parallel jobs for build/test step.
+        NPROC=$(nproc)
+        echo CMAKE_BUILD_PARALLEL_LEVEL=$(($NPROC + 1)) | tee -a $GITHUB_ENV
+      shell: bash
+    - name: Install build and test dependencies
+      run: |
+        apt -y update
+        apt -y install clang-11 cmake ninja-build make perl
+      shell: bash
+    - name: Set Clang as a default toolchain
+      run: |
+        echo CC=clang-11 | tee -a $GITHUB_ENV
+        echo CXX=clang++-11 | tee -a $GITHUB_ENV
+      shell: bash
diff --git a/.github/workflows/sanitizers-testing.yml b/.github/workflows/sanitizers-testing.yml
new file mode 100644
index 00000000..ab8e5dc7
--- /dev/null
+++ b/.github/workflows/sanitizers-testing.yml
@@ -0,0 +1,79 @@
+name: Sanitizers 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/release/2.11,
+  # tarantool/release/2.10, 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: ${{ 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-asan:
+    strategy:
+      fail-fast: false
+      matrix:
+        # XXX: Let's start with only Linux/x86_64
+        BUILDTYPE: [Debug, Release]
+        GC64: [ON, OFF]
+        include:
+          - BUILDTYPE: Debug
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
+          - BUILDTYPE: Release
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
+    runs-on: [self-hosted, regular, Linux, x86_64]
+    name: >
+      LuaJIT with ASan (Linux/x86_64)
+      ${{ matrix.BUILDTYPE }}
+      GC64:${{ matrix.GC64 }}
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          fetch-depth: 0
+          submodules: recursive
+      - name: setup Linux for sanitizers
+        uses: ./.github/actions/setup-sanitizers
+      - name: configure
+        run: >
+          cmake -S . -B ${{ env.BUILDDIR }}
+          -G Ninja
+          ${{ matrix.CMAKEFLAGS }}
+          -DLUAJIT_ENABLE_GC64=${{ matrix.GC64 }}
+          -DLUAJIT_USE_ASAN=ON
+      - name: build
+        run: cmake --build . --parallel
+        working-directory: ${{ env.BUILDDIR }}
+      - name: test
+        env:
+          # Enable as much checks as possible. See more info here:
+          # https://github.com/google/sanitizers/wiki/AddressSanitizerFlags
+          ASAN_OPTIONS: "                    \
+            detect_invalid_pointer_pairs=1:  \
+            detect_leaks=1:                  \
+            detect_stack_use_after_return=1: \
+            dump_instruction_bytes=1:        \
+            heap_profile=0:                  \
+            print_suppressions=0             \
+            symbolize=1:                     \
+            unmap_shadow_on_exit=1:          \
+          "
+        run: cmake --build . --parallel --target LuaJIT-test
+        working-directory: ${{ env.BUILDDIR }}
-- 
2.30.2

Re: [Tarantool-patches] [PATCH luajit 2/2] ci: introduce testing workflow with sanitizers

[Sergey Bronnikov via Tarantool-patches]

Thanks for the patch! LGTM with minor comments below

Sergey

On 7/13/23 00:09, Igor Munkin wrote:
> This commit adds GitHub workflow running all available LuaJIT tests with
> LUAJIT_USE_ASAN option enabled. For now, sanitizers workflow works only
> for Linux/x86_64 as the most scaling setup in our CI. I believe we will
> be able to add other platforms being supported, when ARM64 and macOS

beware that ARM64 is absent in a list of supported platforms [1]

https://clang.llvm.org/docs/AddressSanitizer.html#supported-platforms

<snipped>

> +      - name: test
> +        env:
> +          # Enable as much checks as possible. See more info here:

However, some checks are *disabled*, for example "print_suppressions"

and "heap_profile" are disabled. Is it intentionally?

> +          # https://github.com/google/sanitizers/wiki/AddressSanitizerFlags
there are also LeakSanitizer flags here 
https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer#flags
> +          ASAN_OPTIONS: "                    \
> +            detect_invalid_pointer_pairs=1:  \
> +            detect_leaks=1:                  \
> +            detect_stack_use_after_return=1: \
> +            dump_instruction_bytes=1:        \
> +            heap_profile=0:                  \
> +            print_suppressions=0             \
> +            symbolize=1:                     \
> +            unmap_shadow_on_exit=1:          \
> +          "
> +        run: cmake --build . --parallel --target LuaJIT-test
> +        working-directory: ${{ env.BUILDDIR }}

Re: [Tarantool-patches] [PATCH luajit 2/2] ci: introduce testing workflow with sanitizers

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! See my replies below, and the second version of
the series[1] for updates.

On 13.07.23, Sergey Bronnikov wrote:
> Thanks for the patch! LGTM with minor comments below
> 
> Sergey
> 
> On 7/13/23 00:09, Igor Munkin wrote:
> > This commit adds GitHub workflow running all available LuaJIT tests with
> > LUAJIT_USE_ASAN option enabled. For now, sanitizers workflow works only
> > for Linux/x86_64 as the most scaling setup in our CI. I believe we will
> > be able to add other platforms being supported, when ARM64 and macOS
> 
> beware that ARM64 is absent in a list of supported platforms [1]
> 
> https://clang.llvm.org/docs/AddressSanitizer.html#supported-platforms

Thanks for mentioning: removed this from commit message.

> 
> <snipped>
> 
> > +      - name: test
> > +        env:
> > +          # Enable as much checks as possible. See more info here:
> 
> However, some checks are *disabled*, for example "print_suppressions"
> 
> and "heap_profile" are disabled. Is it intentionally?

"print_suppressions" is disabled intentionally, to not spoil the output
of the test; "heap_profile" is also disabled in Tarantool, and it is
experimental, so I'm afraid introducing flaky failures when enable it.
If you can suggest particular flags to be enabled, feel free to share in
the second version of the series[1].

> 
> > +          # https://github.com/google/sanitizers/wiki/AddressSanitizerFlags
> there are also LeakSanitizer flags here
> https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer#flags
> > +          ASAN_OPTIONS: "                    \
> > +            detect_invalid_pointer_pairs=1:  \
> > +            detect_leaks=1:                  \
> > +            detect_stack_use_after_return=1: \
> > +            dump_instruction_bytes=1:        \
> > +            heap_profile=0:                  \
> > +            print_suppressions=0             \
> > +            symbolize=1:                     \
> > +            unmap_shadow_on_exit=1:          \
> > +          "
> > +        run: cmake --build . --parallel --target LuaJIT-test
> > +        working-directory: ${{ env.BUILDDIR }}

[1]: https://lists.tarantool.org/tarantool-patches/cover.1689925402.git.imun@tarantool.org/T/#t

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit 2/2] ci: introduce testing workflow with sanitizers

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!
The patch LGTM as general, just a single note, also mentioned by Sergey
offline:

Should we also enable LUAJIT_USE_SYSMALLOC for our CI workflow to make
the ASan instrumentation usefull?

On 12.07.23, Igor Munkin wrote:
> This commit adds GitHub workflow running all available LuaJIT tests with
> LUAJIT_USE_ASAN option enabled. For now, sanitizers workflow works only
> for Linux/x86_64 as the most scaling setup in our CI. I believe we will
> be able to add other platforms being supported, when ARM64 and macOS
> runners starvation is defeated. There is also a separate GitHub Action
> introduced for convenient setup of the environment.
> 
> Follows up tarantool/tarantool#5878
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>

<snipped>

> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2] ci: introduce testing workflow with sanitizers

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for your review!

On 13.07.23, Sergey Kaplun wrote:
> Hi, Igor!
> The patch LGTM as general, just a single note, also mentioned by Sergey
> offline:
> 
> Should we also enable LUAJIT_USE_SYSMALLOC for our CI workflow to make
> the ASan instrumentation usefull?

I added the warning in CMake, if LUAJIT_USE_ASAN is enabled without
LUAJIT_USE_SYSMALLOC, and enabled it in CI. See the second version of
the series[1] for updates.

> 
> On 12.07.23, Igor Munkin wrote:
> > This commit adds GitHub workflow running all available LuaJIT tests with
> > LUAJIT_USE_ASAN option enabled. For now, sanitizers workflow works only
> > for Linux/x86_64 as the most scaling setup in our CI. I believe we will
> > be able to add other platforms being supported, when ARM64 and macOS
> > runners starvation is defeated. There is also a separate GitHub Action
> > introduced for convenient setup of the environment.
> > 
> > Follows up tarantool/tarantool#5878
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> 
> <snipped>
> 
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

[1]: https://lists.tarantool.org/tarantool-patches/cover.1689925402.git.imun@tarantool.org/T/#t

-- 
Best regards,
IM