Hi, Maxim,
thanks for the patch! See my comments below.
From: Maksim Tiushev <mandesero@gmail.com> This patch adds CI testing with Valgrind in three scenarios: - 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`.
Motivation behind chosen configurations is not clear.
And what is a difference for "--malloc-fill=0x00"/"--free-fill=0x00"
and "--malloc-fill=0xFF"/"--free-fill=0xFF". Why the fill value
make sense here?
Also, we usually mention an issue number in commit messages.
Cover letter says that the issue behind these patches is
https://github.com/tarantool/tarantool/issues/3705
but commit messages forĀ both patches doesn't mention the issue.
I propose to add "Part of #3705" to the first patch and
"Closes #3705" to the second patch.
This config doesn't have VALGRIND_OPTIONS, is it intentional?--- .github/actions/setup-valgrind/README.md | 12 +++ .github/actions/setup-valgrind/action.yml | 12 +++ .github/workflows/valgrind-testing.yaml | 95 +++++++++++++++++++++++ 3 files changed, 119 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..e7d66a3a --- /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-valgrind + if: ${{ matrix.OS == 'Linux' }} +``` \ No newline at end of file diff --git a/.github/actions/setup-valgrind/action.yml b/.github/actions/setup-valgrind/action.yml new file mode 100644 index 00000000..5c11fdaa --- /dev/null +++ b/.github/actions/setup-valgrind/action.yml @@ -0,0 +1,12 @@ +name: Setup CI environment with Valgrind on Linux +description: Extend setup-linux with Valgrind installation + +runs: + using: composite + steps: + - name: Setup CI environment (Linux) + uses: ./.github/actions/setup-linux + - name: Install Valgrind + run: | + apt -y install valgrind + shell: bash diff --git a/.github/workflows/valgrind-testing.yaml b/.github/workflows/valgrind-testing.yaml new file mode 100644 index 00000000..15038092 --- /dev/null +++ b/.github/workflows/valgrind-testing.yaml @@ -0,0 +1,95 @@ +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 + # We don't test on Linux/ARM64 because the address returned by the + # system allocator may exceed 47 bits. As a result, we are unable to + # allocate memory for `lua_State`. Therefore, testing on this platform + # is currently disabled. + 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
Why JOB_POSTFIX is empty here?+ - BUILDTYPE: Release + CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
Please explain a meaning of specified options.+ - 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 }}