From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id BCF4ED431C2; Fri, 1 Nov 2024 17:17:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BCF4ED431C2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1730470668; bh=HaCCjfjYc4PfxG+3y3qabVnOMNMZ64vPr9f57gNSNj0=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=FPHFnpBc+BNs+LXjGxV5WL3beuqXAOfRiHcBl+2Kzptq8rfw0J3jq5Q4LCTDJ329V G/W7+8tRukWW+MkRty7WyRRw6u2BllZ7WairtBlUzeFe9t3k1ogPwu70/upQMtmxbo gptCNHagbREWZvGOKAMNMYic6iG4rwDmeruo1gUw= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [95.163.41.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 10C9CD431C1 for ; Fri, 1 Nov 2024 17:17:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 10C9CD431C1 Received: by exim-smtp-6c5957b6dd-hl445 with esmtpa (envelope-from ) id 1t6sTD-000000007dI-0esh; Fri, 01 Nov 2024 17:17:47 +0300 Content-Type: multipart/alternative; boundary="------------6Pc7mWxidNaZQTObcCvjpfZM" Message-ID: <8cf43811-f0bc-4acf-b4b2-d197cf28a87e@tarantool.org> Date: Fri, 1 Nov 2024 17:17:46 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: mandesero@gmail.com, tarantool-patches@dev.tarantool.org, skaplun@tarantool.org, m.kokryashkin@tarantool.org References: <20240918085053.150351-1-mandesero@gmail.com> <20240918085053.150351-3-mandesero@gmail.com> In-Reply-To: <20240918085053.150351-3-mandesero@gmail.com> X-Mailru-Src: smtp X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92733C19FC6DB0C8BEE2BF03C37FF8A09DBF7179AED752434182A05F53808504019B012098229074C3DE06ABAFEAF67057BB9EFB4F3E9AF3D9B7CB9E25DAE2DB7A938C1B4F21A4609 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A3295C83650092F9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063725FA9CD6081C82518638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D84D4317F8700A991F043C4E074625D07E18CDBE3B8E7A7028CC7F00164DA146DAFE8445B8C89999728AA50765F7900637BA939FD1B3BAB99B389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC81D471462564A2E19F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C6089696B24BB1D19C0837EA9F3D197644AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C382354B272D6C28B2BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7C30BC1F983723CA3731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A58C7CAF0A3F4C53AD5002B1117B3ED6967D8F97221B3AD4248B25839F35DFE037823CB91A9FED034534781492E4B8EEAD09F854029C6BD0DABDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF77DD89D51EBB7742D3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFF49634BE6D533C9B118108930A0C096CC40B48BF7841AFF7B3321BD0FBE96A05A161E968D9DBB34902643FFCD63D2E1856A90FA27C32A54BCD4126477CC1339C16010914A29A91B1C226CC413062362A913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojfMbdV4AJnq5nFbibPf/3Vg== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D6140F2F0930EB2667D8991417EB218679B82284FAF3BCE3A140C0152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v3 luajit 2/2] ci: add Valgrind testing workflow X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------6Pc7mWxidNaZQTObcCvjpfZM Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, Maxim, thanks for the patch! See my comments below. On 18.09.2024 11:50, mandesero--- via Tarantool-patches wrote: > From: Maksim Tiushev > > 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. > --- > .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 This config doesn't have VALGRIND_OPTIONS, is it intentional? > + - BUILDTYPE: Release > + CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo Why JOB_POSTFIX is empty here? > + - VALGRIND_SCENARIO: full > + VALGRIND_OPTIONS: --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose Please explain a meaning of specified options. > + 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 }} --------------6Pc7mWxidNaZQTObcCvjpfZM Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi, Maxim,

thanks for the patch! See my comments below.


On 18.09.2024 11:50, mandesero--- via Tarantool-patches wrote:
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.

---
 .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
This config doesn't have VALGRIND_OPTIONS, is it intentional?
+          - BUILDTYPE: Release
+            CMAKEFLAGS: -DCMAKE_BUILD_TYPE=RelWithDebInfo
Why JOB_POSTFIX is empty here?
+          - VALGRIND_SCENARIO: full
+            VALGRIND_OPTIONS: --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose
Please explain a meaning of specified options.
+            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 }}
--------------6Pc7mWxidNaZQTObcCvjpfZM--