[Tarantool-patches] [PATCH luajit v2 5/5] ci: introduce testing workflow with sanitizers
Igor Munkin
imun at tarantool.org
Wed Jul 26 17:53:32 MSK 2023
Sergey,
Thanks for your review! See my answers inline.
On 24.07.23, Sergey Bronnikov wrote:
> Thanks for the patch!
>
> See my comments below.
>
>
> Sergey
>
> On 7/21/23 11:12, Igor Munkin wrote:
>
<snipped>
> > diff --git a/.github/actions/setup-sanitizers/README.md b/.github/actions/setup-sanitizers/README.md
> > new file mode 100644
> > index 00000000..3aa9e214
> > --- /dev/null
> > +++ b/.github/actions/setup-sanitizers/README.md
> > @@ -0,0 +1,12 @@
> > +# Setup environment for sanitizers on Linux
> > +
> > +Action setups the environment on Linux runners (install requirements, setup the
> > +workflow environment, etc) for testing with sanitizers enabled.
> > +
> > +## How to use Github Action from Github workflow
> > +
> > +Add the following code to the running steps before LuaJIT configuration:
> > +```
> > +- uses: ./.github/actions/setup-sanitizers
> > + if: ${{ matrix.OS == 'Linux' }}
>
> Nit: I would left a comment why OSes is limited by Linux only.
>
> You actually said it in commit message ("I believe we will
> be able to add other platforms being supported, when macOS runners
> starvation is defeated.").
I have literally written it here (see the first paragraph at the
beginning of the README.md).
| Action setups the environment on Linux runners (install requirements,
| setup the workflow environment, etc) for testing with sanitizers
| enabled.
>
> > +```
<snipped>
> > 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
> > @@ -0,0 +1,89 @@
<snipped>
> > +jobs:
> > + test-asan:
> > + strategy:
> > + fail-fast: false
> > + matrix:
> > + # XXX: Let's start with only Linux/x86_64
> > + BUILDTYPE: [Debug, Release]
> > + 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]
> > + name: >
> > + LuaJIT with ASan (Linux/x86_64)
> > + ${{ matrix.BUILDTYPE }}
> > + GC64:ON SYSMALLOC:ON
> > + steps:
> > + - uses: actions/checkout at v3
> > + with:
> > + fetch-depth: 0
> > + submodules: recursive
> > + - name: setup Linux for sanitizers
> > + uses: ./.github/actions/setup-sanitizers
> > + - 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
> Nit: I suppose it would be easy read paragraph above if indent it by "-S",
> not by "cmake".
We have such alignment in all workflow files. The idea is nice, but I
see no reason to touch it now. Ignoring.
> > + - 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.
> > + 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: \
> > + "
>
> This options are used in CI and doesn't present in CMake, so ASAN will work
> different locally and in CI.
>
> Is it expected?
As we discussed offline, it's not a trivial task to implement,
considering integration testing with Tarantool. Since, this is not a
popular configuration to be manually tested (until you poisoned the code
with a leak), I suggest to return to this later. From my side, I have
several ideas to try, so I'll share the results when I have some.
>
> > + run: cmake --build . --parallel --target LuaJIT-test
> > + working-directory: ${{ env.BUILDDIR }}
--
Best regards,
IM
More information about the Tarantool-patches
mailing list