Re: [Tarantool-patches] [PATCH luajit v2 5/5] ci: introduce testing workflow with sanitizers

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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@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