Hi, Evgeniy!
Thanks for the fixes!
Please consider the following comments:
1. please fix an action name in the
.github/actions/setup-sanitizers-linux/README.md
s/setup-sanitizers/setup-sanitizers-linux/
should we set CMAKE_PREFIX_PATH in
.github/actions/setup-sanitizers-linux/action.yml like we do in
the macos version.
the comment "# Try to install" is obvious and excess
2. there is inconsistency in the .github/actions/setup-sanitizers-macos/README.md and implementation:
README says: "Requires input: cc_name" but the current implementation has a default (and `required: false`).
In action.yml, `CMAKE_C_COMPILER=clang-21` (the name, not the full path) and `CMAKE_PREFIX_PATH` are set,
and `-DCMAKE_C_COMPILER=clang-21` is passed to the workflow. CMake looks for the compiler in the `PATH` to
detect it - `CMAKE_PREFIX_PATH` has no effect on this. Either the required compiler is already in the runner's `PATH`
(in which case `.github/actions/setup-macos` is unnecessary, as it doesn't add anything to the `PATH`),
or the configuration will fail. You should explicitly add `$(brew --prefix llvm@21)/bin` to the `PATH`.
3. I don't like that we have three copies of ASAN_OPTIONS in the same workflow. It is better to fix this.
4. .github/actions/setup-sanitizers-linux/action.yml:
why CC was renamed to CMAKE_C_COMPILER? I would leave CC env var. CMAKE_C_COMPILER is a CMake option name name, GH action
knows nothing about CMake.
Also, please fix CMake name: s/cmake/CMake/
Sergey
Agree, let's keep as is.Hi, Sergey! Thanks for review!Please, see my answers below.Fixes applied and the branch is force pushed.From: Sergey Bronnikov <sergeyb@tarantool.org>
To: Evgeniy Temirgaleev <e.temirgaleev@tarantool.org>, Sergey Kaplun <skaplun@tarantool.org>
Cc:tarantool-patches@dev.tarantool.org
Date: Thursday, August 27, 2026 3:07 PM +03:00
Hi, Evgeniy,
thanks for the patch! See my comments below.
Sergey
On 8/6/26 15:47, Evgeniy Temirgaleev wrote:From: Temir Galeev <temir.galeev@bk.ru> The arm64 and x86_64 architectures with clang/gcc compiler were added to the matrix. --- .../README.md | 0 .../action.yml | 8 +- .../actions/setup-sanitizers-macos/README.md | 15 +++ .../actions/setup-sanitizers-macos/action.yml | 73 +++++++++++++ .github/workflows/sanitizers-testing.yml | 101 ++++++++++++++++-- 5 files changed, 186 insertions(+), 11 deletions(-) rename .github/actions/{setup-sanitizers => setup-sanitizers-linux}/README.md (100%) rename .github/actions/{setup-sanitizers => setup-sanitizers-linux}/action.yml (76%) create mode 100644 .github/actions/setup-sanitizers-macos/README.md create mode 100644 .github/actions/setup-sanitizers-macos/action.yml diff --git a/.github/actions/setup-sanitizers/README.md b/.github/actions/setup-sanitizers-linux/README.md similarity index 100% rename from .github/actions/setup-sanitizers/README.md rename to .github/actions/setup-sanitizers-linux/README.md diff --git a/.github/actions/setup-sanitizers/action.yml b/.github/actions/setup-sanitizers-linux/action.yml similarity index 76% rename from .github/actions/setup-sanitizers/action.yml rename to .github/actions/setup-sanitizers-linux/action.yml index 8642d553..18f5a75d 100644 --- a/.github/actions/setup-sanitizers/action.yml +++ b/.github/actions/setup-sanitizers-linux/action.yml @@ -20,13 +20,17 @@ runs: - name: Install build and test dependencies run: | apt -y update + echo Available compilers: + export CC_FAMILY=`echo ${CC_NAME} | sed 's/-.*$//'` + apt list | grep -Pe "^${CC_FAMILY}-[0-9]+/" + # Try to installHonestly, I don't get why we should available compilers on each run.
I think this information is important for some cases to have it on hand. The example was present in the thread above: https://lists.tarantool.org/pipermail/tarantool-patches/2026-August/030774.htmlWhy we cannot
hardcode compiler here?
This patch is not intended to refactor linux sanitizers action, so the existing solution is used.Only info about available compilers was added:«Also, the 'setup-sanitizers-macos' action supplied withthe 'list available compilers' commands in one of it's job.It helps to get the answer to the question:'Which compiler we can select just now with our current environment?'The 'setup-sanitizers-linux' build job extended with such commands also.»s/cmake/CMake/?apt -y install ${CC_NAME} libstdc++-10-dev cmake ninja-build make perl shell: bash env: CC_NAME: ${{ inputs.cc_name }} - - name: Set specific C compiler as a default toolchain + - name: Set specific C compiler as a default toolchain for cmakeFixed.s/tee -a/>>/ (feel free to ignore, previously tee was used)run: | - echo CC=${CC_NAME} | tee -a $GITHUB_ENV + echo CMAKE_C_COMPILER=${CC_NAME} | tee -a $GITHUB_ENVThe ‘tee -a’ method allows the programmer to see the step’s result of the env definition. It’s used in the ‘setup’, ‘setup-linux’, ‘setup-macos’ actions and some workflows already. I think it’s a good approach to use. Also, there is an empty grep for the ‘>> $GITHUB_ENV’ method in our scripts.If there are the strong reasons to change it, I think it must be done in all places and in the separate refactoring patch.
shell: bash env: CC_NAME: ${{ inputs.cc_name }} diff --git a/.github/actions/setup-sanitizers-macos/README.md b/.github/actions/setup-sanitizers-macos/README.md new file mode 100644 index 00000000..9ac5eb38 --- /dev/null +++ b/.github/actions/setup-sanitizers-macos/README.md @@ -0,0 +1,15 @@ +# Setup environment for sanitizers on macOS + +Action setups the environment on macOS runners (install requirements, setup the +workflow environment, etc) for testing with sanitizers enabled. + +Requires input: +- cc_name as versioned C compiler: gcc-ver or clang-ver. + +## How to use Github Action from Github workflow + +Add the following code to the running steps before LuaJIT configuration: +``` +- uses: ./.github/actions/setup-sanitizers-macos + if: ${{ matrix.OS == 'macOS' }} +``` diff --git a/.github/actions/setup-sanitizers-macos/action.yml b/.github/actions/setup-sanitizers-macos/action.yml new file mode 100644 index 00000000..9836ea03 --- /dev/null +++ b/.github/actions/setup-sanitizers-macos/action.yml @@ -0,0 +1,73 @@ +name: Setup CI environment for testing with sanitizers on macOS +description: Common part to tweak macOS CI runner environment for sanitizers +inputs: + cc_name: + description: C compiler name (for example, gcc-12) + required: false + default: clang-21 +runs: + using: composite + steps: + - name: Get compiler version from cc_name + shell: bash + env: + CC_NAME: ${{ inputs.cc_name }} + run: | + echo CC_VERSION=`echo ${CC_NAME} | sed 's/.*-//'` | tee -a $GITHUB_ENV + - name: Get brew formula from cc_name + shell: bash + env: + CC_FORMULA_NAME: |- + ${{ case( + startsWith(inputs.cc_name, 'gcc'), 'gcc', + startsWith(inputs.cc_name, 'clang'), 'llvm', + 'MISCONFIG' + ) }} + run: | + echo CC_FORMULA=${CC_FORMULA_NAME}@${CC_VERSION} | tee -a $GITHUB_ENV + echo Available formulas: `brew search ${CC_FORMULA_NAME}` + - name: Setup CI environment + uses: ./.github/actions/setup + - name: Set CMAKE_BUILD_PARALLEL_LEVEL + shell: bash + run: | + # Set CMAKE_BUILD_PARALLEL_LEVEL environment variable to + # limit the number of parallel jobs for build/test step. + NPROC=$(sysctl -n hw.logicalcpu 2>/dev/null) + echo CMAKE_BUILD_PARALLEL_LEVEL=$(($NPROC + 1)) | tee -a $GITHUB_ENV + - name: Set MACOSX_DEPLOYMENT_TARGERT + shell: bash + run: | + # Set required MACOSX_DEPLOYMENT_TARGERT environment + # variable for Makefile.original build. + # See https://github.com/LuaJIT/LuaJIT/issues/484, + # https://github.com/LuaJIT/LuaJIT/issues/653. + echo MACOSX_DEPLOYMENT_TARGET=$(sw_vers -productVersion) | tee -a $GITHUB_ENV + - name: Install build and test dependencies + shell: bash + run: | + # Install brew using the command from Homebrew repository + # instructions: https://github.com/Homebrew/install. + # XXX: 'echo' command below is required since brew + # installation script obliges the one to enter a newline + # for confirming the installation via Ruby script. + brew update || + echo | /usr/bin/ruby -e "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/master/install)" + # Try to install the packages either upgrade it to avoid + # of fails if the package already exists with the previous + # version. + brew install --force ${CC_FORMULA} cmake make ninja perl || + brew upgrade ${CC_FORMULA} cmake make ninja perl + - name: Set specific C compiler as a default toolchain for cmake + shell: bash + env: + CC_NAME: ${{ inputs.cc_name }} + run: | + echo CMAKE_C_COMPILER=${CC_NAME} | tee -a $GITHUB_ENV + echo CMAKE_PREFIX_PATH="`brew --prefix ${CC_FORMULA}`" | tee -a $GITHUB_ENV + - name: Log installed compilers + shell: bash + run: | + echo default clang: `clang --version` + echo default gcc: `gcc --version` + echo cmake compiler: `${CMAKE_PREFIX_PATH}/bin/${CMAKE_C_COMPILER} --version` diff --git a/.github/workflows/sanitizers-testing.yml b/.github/workflows/sanitizers-testing.yml index 4bf7d023..fe550b81 100644 --- a/.github/workflows/sanitizers-testing.yml +++ b/.github/workflows/sanitizers-testing.yml @@ -31,17 +31,41 @@ jobs: strategy: fail-fast: false matrix: - # XXX: Let's start with only Linux/x86_64 + ARCH: [ARM64, x86_64] BUILDTYPE: [Debug, Release] - CC: [gcc-10, clang-11] + OS: [Linux, macOS] + # There are different top-level versions available for Linux and macOS runners. + CC: [gcc-10, clang-11, gcc-15, clang-21] 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] + exclude: + # On current runners with Linux/ARM64 environment and + # with LUAJIT_USE_SYSMALLOC=ON the system allocator returns addresses + # with 48-bit set. Thus checkptrGC() fails with new Lua state pointer + # and luajit fails to start with 'cannot create state: not enough memory' + # error. So, we exclude these cases. + - ARCH: ARM64 + OS: Linux + # Exclude nonsuitable OS/compiler pairs. + - OS: macOS + CC: gcc-10 + - OS: macOS + CC: clang-11 + - OS: Linux + CC: gcc-15 + - OS: Linux + CC: clang-21 + # Exclude macOS/ARM64/gcc case due to some tests are failed. + # Details: https://github.com/tarantool/tarantool/issues/13018 + - ARCH: ARM64 + OS: macOS + CC: gcc-15 + runs-on: [self-hosted, regular, '${{ matrix.OS }}', '${{ matrix.ARCH }}'] name: > - LuaJIT with ASan and UBSan (Linux/x86_64) + LuaJIT with ASan and UBSan (${{ matrix.OS }}/${{ matrix.ARCH }}) ${{ matrix.BUILDTYPE }} CC:${{ matrix.CC }} GC64:ON SYSMALLOC:ON @@ -51,7 +75,13 @@ jobs: fetch-depth: 0 submodules: recursive - name: setup Linux for sanitizers - uses: ./.github/actions/setup-sanitizers + if: ${{ matrix.OS == 'Linux' }} + uses: ./.github/actions/setup-sanitizers-linux + with: + cc_name: ${{ matrix.CC }} + - name: setup macOS for sanitizers + if: ${{ matrix.OS == 'macOS' }} + uses: ./.github/actions/setup-sanitizers-macos with: cc_name: ${{ matrix.CC }} - name: configure @@ -70,18 +100,46 @@ jobs: cmake -S . -B ${{ env.BUILDDIR }} -G Ninja ${{ matrix.CMAKEFLAGS }} + -DCMAKE_C_COMPILER=${CMAKE_C_COMPILER} + -DCMAKE_PREFIX_PATH=${CMAKE_PREFIX_PATH} -DLUAJIT_ENABLE_GC64=ON -DLUAJIT_USE_ASAN=ON -DLUAJIT_USE_SYSMALLOC=ON -DLUAJIT_USE_UBSAN=ON + - name: Check for possible compiler misconfig + working-directory: ${{ env.BUILDDIR }} + env: + CC_NAME: ${{ matrix.CC }} + run: grep CMakeCache.txt -e CMAKE_C_COMPILER:STRING | grep ${CC_NAME} - name: build run: cmake --build . --parallel working-directory: ${{ env.BUILDDIR }} - - name: test + + # Enable as much checks as possible. See more info here:You say about enabling ASAN features, but some features are disabled below,
please explain why these features are disabled.
already enabled by default+ # https://github.com/google/sanitizers/wiki/AddressSanitizerFlags, + # https://github.com/google/sanitizers/wiki/SanitizerCommonFlags. + - name: setup sanitizer options for Linux + if: ${{ matrix.OS == 'Linux' }} + env: + ASAN_OPTIONS: " \ + detect_invalid_pointer_pairs=1: \ + detect_leaks=1: \disabled by default+ detect_stack_use_after_return=1: \ + dump_instruction_bytes=1: \ + heap_profile=0: \Why disabled?+ print_suppressions=0: \enabled by default+ symbolize=1: \The patch doesn’t enable ASAN for Linux, so this part isn’t changed: the ASAN options for Linux is used as is.The ASAN options for macOS is just a copy of the Linux options with one exception. The detect_leaks was disabled with the explanation in a comment.The full option investigation and selection in not the main goal of the patch. The patch enables ASAN for macOS and it’s truly enabled for the options selected.I agree that the actualization of options for both Linux and macOS is a valuable job. I suggest to make a ticket for it.s/tee -a/>>/ (the same thing but usually used in Github documentation)+ unmap_shadow_on_exit=1: \ + " + UBSAN_OPTIONS: " + print_stacktrace=1 \ + " + run: | + echo ASAN_OPTIONS=${ASAN_OPTIONS} | tee -a $GITHUB_ENV + echo UBSAN_OPTIONS=${UBSAN_OPTIONS} | tee -a $GITHUB_ENVAnswered above.s/tee -a/>>/+ - name: setup sanitizer options for macOS (common) + if: ${{ matrix.OS == 'macOS' && matrix.ARCH != 'ARM64' && matrix.CC != 'clang-21' }} env: - # Enable as much checks as possible. See more info here: - # https://github.com/google/sanitizers/wiki/AddressSanitizerFlags, - # https://github.com/google/sanitizers/wiki/SanitizerCommonFlags. ASAN_OPTIONS: " \ detect_invalid_pointer_pairs=1: \ detect_leaks=1: \ @@ -95,5 +153,30 @@ jobs: UBSAN_OPTIONS: " print_stacktrace=1 \ " + run: | + echo ASAN_OPTIONS=${ASAN_OPTIONS} | tee -a $GITHUB_ENV + echo UBSAN_OPTIONS=${UBSAN_OPTIONS} | tee -a $GITHUB_ENVThe same.the same questions as above. Also, can we avoid duplication?+ - name: setup sanitizer options for macOS (ARM64/clang-21) + if: ${{ matrix.OS == 'macOS' && matrix.ARCH == 'ARM64' && matrix.CC == 'clang-21' }} + # detect_leaks is disabled due to some tests build is failed. + # Details: https://github.com/tarantool/tarantool/issues/13019 + env: + ASAN_OPTIONS: " \ + detect_invalid_pointer_pairs=1: \ + detect_leaks=0: \ + detect_stack_use_after_return=1: \ + dump_instruction_bytes=1: \ + heap_profile=0: \ + print_suppressions=0: \ + symbolize=1: \ + unmap_shadow_on_exit=1: \May be we can use the file to accumulate the options and to update it in a specific steps. I suggest this task to the new ticket ‘ci: actualization of the ASAN options for Linux and macOS’ also.s/tee -a/>>/+ " + UBSAN_OPTIONS: " + print_stacktrace=1 \ + " + run: | + echo ASAN_OPTIONS=${ASAN_OPTIONS} | tee -a $GITHUB_ENV + echo UBSAN_OPTIONS=${UBSAN_OPTIONS} | tee -a $GITHUB_ENVAnswered above.+ - name: test run: cmake --build . --parallel --target LuaJIT-test working-directory: ${{ env.BUILDDIR }}The changes applied:
diff --git a/.github/actions/setup-sanitizers-linux/action.yml b/.github/actions/setup-sanitizers-linux/action.ymlindex 9744e5dd..19314dca 100644--- a/.github/actions/setup-sanitizers-linux/action.yml+++ b/.github/actions/setup-sanitizers-linux/action.yml@@ -28,7 +28,7 @@ runs:shell: bashenv:CC_NAME: ${{ inputs.cc_name }}- - name: Set specific C compiler as a default toolchain for cmake+ - name: Set specific C compiler as a default toolchain for CMake.run: |echo CMAKE_C_COMPILER=${CC_NAME} | tee -a $GITHUB_ENVshell: bashdiff --git a/.github/actions/setup-sanitizers-macos/action.yml b/.github/actions/setup-sanitizers-macos/action.ymlindex d2160faa..740943af 100644--- a/.github/actions/setup-sanitizers-macos/action.yml+++ b/.github/actions/setup-sanitizers-macos/action.yml@@ -35,7 +35,7 @@ runs:# of fails if the package already exists with the previous# version.brew install --force ${CC_FORMULA} || brew upgrade ${CC_FORMULA}- - name: Set specific C compiler as a default toolchain for cmake+ - name: Set specific C compiler as a default toolchain for CMake.shell: bashenv:CC_NAME: ${{ inputs.cc_name }}--
Best regards,Evgeniy Temirgaleev