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

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

Hi, Maksim!
Thanks for the fixes!
Please consider my comments below.

On 02.12.24, mandesero@gmail.com wrote:
> From: Maksim Tiushev <mandesero@gmail.com>

<snipped>

> +          - VALGRIND_SCENARIO: full
> +            VALGRIND_OPTS: --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose --error-exitcode=1
> +            JOB_POSTFIX: "leak-check: full"
> +          # The use of `0x00` and `0xFF` for memory fill helps to detect dirty
> +          #  reads. `0x00` mimics zero-initialized memory, which can mask some
> +          #  uninitialized memory usage. `0xFF` fills memory with a non-zero
> +          #  values to make such errors easier to spot.
> +          - VALGRIND_SCENARIO: malloc-free-fill-0x00
> +            VALGRIND_OPTS: --leak-check=no --malloc-fill=0x00 --free-fill=0x00 --verbose --error-exitcode=1
> +            JOB_POSTFIX: "malloc/free-fill: 0x00"
> +          - VALGRIND_SCENARIO: malloc-free-fill-0xff
> +            VALGRIND_OPTS: --leak-check=no --malloc-fill=0xff --free-fill=0xff --verbose --error-exitcode=1

I suppose that common options for all suites may be placed inside the
`${test_command}` during CMake configuration. Thus we don't forget about
them, and it is more convenient for local testing -- I shouldn't add
VALGRIND_OPTS="--error-exitcode=1 " each time. And this approach is more
robust (see the comment below).

> +            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_OPTS: ${{ matrix.VALGRIND_OPTS }}

I suppose this env should be provided for test step, so these options
are actually used [1]:

| Run cmake --build . --parallel --target LuaJIT-test
|   cmake --build . --parallel --target LuaJIT-test
|   shell: /usr/bin/bash -e {0}
|   env:
|     BUILDDIR: /opt/actions-runner/_work/_temp/build-12086657821
|     CMAKE_BUILD_PARALLEL_LEVEL: 9

Locally I've got errors related to each of our test suites (except
tarantool-c-tests):

| VALGRIND_OPTS="--error-exitcode=1 " ctest -V -L LuaJIT-tests
| ...
| 2: ==16791== ERROR SUMMARY: 4749 errors from 69 contexts (suppressed: 62676 from 164)
| 2/2 Test #2: test/LuaJIT-tests ................***Failed   64.23 sec
|
| VALGRIND_OPTS="--error-exitcode=1 " ctest -V -L lua-Harness-tests
| ..
| 22% tests passed, 40 tests failed out of 51
|
| VALGRIND_OPTS="--error-exitcode=1 " ctest -V -L PUC-Rio-Lua-5.1-tests
| ...
| 4: ==18453== ERROR SUMMARY: 78 errors from 14 contexts (suppressed: 363053 from 81)
| 2/2 Test #4: test/PUC-Rio-Lua-5.1-tests .......***Failed  150.28 sec
|
| VALGRIND_OPTS="--error-exitcode=1 " ctest -V -L tarantool-tests
| ...
| 85% tests passed, 27 tests failed out of 175

I suppose that most of these errors are fixed with additional
suppressions in the <src/lj.supp> since they are related to
`str_fastcmp`, etc. Also, there are some of them that require
investigation.

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

[1]: https://github.com/tarantool/luajit/actions/runs/12086657821/job/33706418395#step:6:1

-- 
Best regards,
Sergey Kaplun