Re: [Tarantool-patches] [PATCH v2 luajit 2/3] ci: add Valgrind testing workflow

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

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

Hi, Maxim,

thanks for the patch! See comments below:

On 12.09.2024 13:21, mandesero--- via Tarantool-patches wrote:
> From: Maksim Tiushev<mandesero@gmail.com>
>
> This patch adds CI testing with Valgrind in three scenarios:
Patch adds a *workflow* with testing under Valgrind...
>    - Full checks enabled.
>    - No leak checks, with memory fill set to `--malloc-fill=0x00`
>      and `--free-fill=0x00`.
>    - No leak checks, with memory fill set to `--malloc-fill=0xFF`
>      and `--free-fill=0xFF`.
> ---
>   .github/actions/setup-valgrind/README.md  | 12 +++
>   .github/actions/setup-valgrind/action.yml | 19 +++++
>   .github/workflows/valgrind-testing.yaml   | 91 +++++++++++++++++++++++
>   3 files changed, 122 insertions(+)
>   create mode 100644 .github/actions/setup-valgrind/README.md
>   create mode 100644 .github/actions/setup-valgrind/action.yml
>   create mode 100644 .github/workflows/valgrind-testing.yaml
>
> diff --git a/.github/actions/setup-valgrind/README.md b/.github/actions/setup-valgrind/README.md
> new file mode 100644
> index 00000000..fabd5af1
> --- /dev/null
> +++ b/.github/actions/setup-valgrind/README.md
> @@ -0,0 +1,12 @@
> +# Setup environment for Valgrind on Linux
> +
> +Action setups the environment on Linux runners (install requirements, setup the
> +workflow environment, etc) for testing with Valgrind.
> +
> +## How to use Github Action from Github workflow
> +
> +Add the following code to the running steps before LuaJIT configuration:
> +```
> +- uses: ./.github/actions/setup-linux
s/setup-linux/setup-valgrind/
> +  if: ${{ matrix.OS == 'Linux' }}
> +```
> diff --git a/.github/actions/setup-valgrind/action.yml b/.github/actions/setup-valgrind/action.yml
> new file mode 100644
> index 00000000..4f6cfba4
> --- /dev/null
> +++ b/.github/actions/setup-valgrind/action.yml
> @@ -0,0 +1,19 @@
> +name: Setup CI environment for Valgrind on Linux
> +description: Common part to tweak Linux CI runner environment
copy-pasted?
> +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
Do you really need this for setup Valgrind?
> +      shell: bash
> +    - name: Install build and test dependencies
> +      run: |
> +        apt -y update
> +        apt -y install cmake gcc make ninja-build perl valgrind
Why do you need gcc/cmake/make etc if an action about Valgrind setup?
> +      shell: bash
> diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml
> new file mode 100644
> index 00000000..693799ea
> --- /dev/null
> +++ b/.github/workflows/valgrind-testing.yaml
> @@ -0,0 +1,91 @@
> +name: Valgrind 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-valgrind:
> +    strategy:
> +      fail-fast: false
> +      matrix:
> +        # XXX: Let's start with only Linux/x86_64
> +        BUILDTYPE: [Debug, Release]
> +        VALGRIND_SCENARIO: [full, malloc-free-fill-0x00, malloc-free-fill-0xff]
> +        include:
> +          - BUILDTYPE: Debug
> +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
> +          - BUILDTYPE: Release
> +            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
> +          - VALGRIND_SCENARIO: full
> +            VALGRIND_OPTIONS: --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose
> +            JOB_POSTFIX: "leak-check: full"
> +          - VALGRIND_SCENARIO: malloc-free-fill-0x00
> +            VALGRIND_OPTIONS: --leak-check=no --malloc-fill=0x00 --free-fill=0x00
> +            JOB_POSTFIX: "malloc/free-fill: 0x00"
> +          - VALGRIND_SCENARIO: malloc-free-fill-0xff
> +            VALGRIND_OPTIONS: --leak-check=no --malloc-fill=0xff --free-fill=0xff
> +            JOB_POSTFIX: "malloc/free-fill: 0xff"
> +    runs-on: [self-hosted, regular, Linux, x86_64]
> +    name: >
> +      LuaJIT with Valgrind (Linux/x86_64)
> +      ${{ matrix.BUILDTYPE }}
> +      CC: gcc
> +      GC64:ON SYSMALLOC:ON
> +      ${{ matrix.JOB_POSTFIX }}
> +    steps:
> +      - uses: actions/checkout@v4
> +        with:
> +          fetch-depth: 0
> +          submodules: recursive
> +      - name: setup Linux for Valgrind
> +        uses: ./.github/actions/setup-valgrind
> +      - name: configure
> +        # XXX: LuaJIT configuration requires a couple of tweaks:
> +        # LUAJIT_USE_SYSMALLOC=ON: 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.
> +        # LUAJIT_ENABLE_GC64=ON: 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.
> +        env:
> +          VALGRIND_OPTIONS: ${{ matrix.VALGRIND_OPTIONS }}
> +        run: >
> +          cmake -S . -B ${{ env.BUILDDIR }}
> +          -G Ninja
> +          ${{ matrix.CMAKEFLAGS }}
> +          -DLUAJIT_USE_VALGRIND=ON
> +          -DLUAJIT_ENABLE_GC64=ON
> +          -DLUAJIT_USE_SYSMALLOC=ON
> +      - name: build
> +        run: cmake --build . --parallel
> +        working-directory: ${{ env.BUILDDIR }}
> +      - name: test
> +        run: cmake --build . --parallel --target LuaJIT-test
> +        working-directory: ${{ env.BUILDDIR }}

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