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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Igor!
Thanks for the patch!
The patch is generally LGTM, after fixing Sergey's comments. But I'm
really concerned about the fact, that the CI don't show any error for
lj-1024-* [1], lj-128-* [2] tests, where we have obvious memleak, since
`os.exit()` is used instead of `test:done(true)` for these tests.

On 21.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 macOS runners
> starvation is defeated. There is also a separate GitHub Action
> introduced for convenient setup of the environment.
> 
> Besided, internal LuaJIT memory allocator is not instrumented yet

Typo? s/Besided/Besides/
Typo: s/internal ... allocator/the internal ... allocator/

> unfortunately, so to find any memory faults it's better to build LuaJIT
> with system provided memory allocator (i.e. run CMake configuration
> phase with -DLUAJIT_USE_SYSMALLOC=ON). However, LUAJIT_USE_SYSMALLOC
> cannot be enabled on x64 without GC64, since realloc usually doesn't
> return addresses in the right address range. For more info, see root
> CMakeLists.txt.
> 
> 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    | 89 +++++++++++++++++++++

<snipped>

> 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

Do we need clang++ for LuaJIT? Why?

> +      shell: bash
> diff --git a/.github/workflows/sanitizers-testing.yml b/.github/workflows/sanitizers-testing.yml
> new file mode 100644
> index 00000000..6c345108
> --- /dev/null
> +++ b/.github/workflows/sanitizers-testing.yml

<snipped>

> +      - name: configure
> +        run: >
> +          cmake -S . -B ${{ env.BUILDDIR }}
> +          -G Ninja
> +          ${{ matrix.CMAKEFLAGS }}
> +          -DLUAJIT_USE_ASAN=ON
> +          # XXX: Unfortunately, internal LuaJIT memory allocator
> +          # is not instrumented yet, so to find any memory errors
> +          # it's better to build LuaJIT with system provided
> +          # memory allocator (i.e. run CMake configuration phase
> +          # with -DLUAJIT_USE_SYSMALLOC=ON). For more info, see
> +          # root CMakeLists.txt.
> +          -DLUAJIT_USE_SYSMALLOC=ON
> +          # XXX: LUAJIT_USE_SYSMALLOC cannot be enabled on x64
> +          # without GC64, since realloc usually doesn't return
> +          # addresses in the right address range. For more info,
> +          # see root CMakeLists.txt.
> +          -DLUAJIT_ENABLE_GC64=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.

Please, also add the link to the [1], since heap_profile is common flag
(same for print_suppressions). Also, it is disabled by default, why do
we need to forcify it to false?

> +          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
> 

[1]: https://github.com/tarantool/luajit/actions/runs/5619185186/job/15225900726#step:6:673
[2]: https://github.com/tarantool/luajit/actions/runs/5619185186/job/15225900726#step:6:638
[3]: https://github.com/google/sanitizers/wiki/SanitizerCommonFlags

-- 
Best regards,
Sergey Kaplun