[Tarantool-patches] [PATCH luajit 0/2] Disable strcmp optimizations in Valgrind build

[Tarantool-patches] [PATCH luajit 0/2] Disable strcmp optimizations in Valgrind build

[mandesero--- via Tarantool-patches]

From: mandesero <mandesero@gmail.com>

In this Patchset, the optimization of strcmp is disabled during the LuaJIT build under Valgrind.
The string comparison is done byte-by-byte. Additionally, the memcmp functions, where strings
are compared, have been replaced to disable any potential internal optimizations.

Branch: https://github.com/tarantool/luajit/tree/mandesero/lj-3705-turn-off-strcmp-opt-in-debug
Issue: https://github.com/tarantool/tarantool/issues/3705

Some tests are failing under Valgrind due to the timer profiler (SIGPROF) or upon reaching
the maximum test duration, so they have been disabled.

mandesero (2):
  c: disable strcmp optimization in Valgrind build
  cmake: running tests under Valgrind, disable tests that failed under
    Valgrind

 .github/actions/setup-sanitizers/action.yml |  2 +-
 .github/workflows/sanitizers-testing.yml    | 53 +++++++++++++++++++++
 src/lj_no_str_opt.supp                      | 16 +++++++
 src/lj_str.c                                | 43 ++++++++++++++++-
 test/CMakeLists.txt                         |  6 ++-
 test/tarantool-tests/CMakeLists.txt         | 17 +++++++
 6 files changed, 133 insertions(+), 4 deletions(-)
 create mode 100644 src/lj_no_str_opt.supp

-- 
2.34.1

[Tarantool-patches] [PATCH luajit 1/2] c: disable strcmp optimization in Valgrind build

[mandesero--- via Tarantool-patches]

From: mandesero <mandesero@gmail.com>

---
 src/lj_str.c | 43 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/src/lj_str.c b/src/lj_str.c
index 321e8c4f..00cced03 100644
--- a/src/lj_str.c
+++ b/src/lj_str.c
@@ -26,10 +26,29 @@ static LJ_AINLINE int str_fastcmp(const char *a, const char *b, MSize len)
 
 /* -- String helpers ------------------------------------------------------ */
 
+#if LUAJIT_USE_VALGRIND
+int lj_str_cmp_no_opt(const char *a, const char *b, MSize len)
+{
+  for (MSize i = 0; i < len; i++)
+    if (a[i] != b[i])
+      return 1;
+  return 0;
+}
+#endif
+
 /* Ordered compare of strings. Assumes string data is 4-byte aligned. */
 int32_t LJ_FASTCALL lj_str_cmp(GCstr *a, GCstr *b)
 {
   MSize i, n = a->len > b->len ? b->len : a->len;
+#if LUAJIT_USE_VALGRIND
+  const uint8_t *sa = (const uint8_t *)strdata(a);
+  const uint8_t *sb = (const uint8_t *)strdata(b);
+  
+  for (i = 0; i < n; i++) {
+    if (sa[i] != sb[i])
+      return (uint8_t)sa[i] < (uint8_t)sb[i] ? -1 : 1;
+  }
+#else
   for (i = 0; i < n; i += 4) {
     /* Note: innocuous access up to end of string + 3. */
     uint32_t va = *(const uint32_t *)(strdata(a)+i);
@@ -46,6 +65,7 @@ int32_t LJ_FASTCALL lj_str_cmp(GCstr *a, GCstr *b)
       return va < vb ? -1 : 1;
     }
   }
+#endif
   return (int32_t)(a->len - b->len);
 }
 
@@ -53,6 +73,12 @@ int32_t LJ_FASTCALL lj_str_cmp(GCstr *a, GCstr *b)
 static LJ_AINLINE int str_fastcmp(const char *a, const char *b, MSize len)
 {
   MSize i = 0;
+#if LUAJIT_USE_VALGRIND
+  for (i = 0; i < len; i++) {
+    if (a[i] != b[i])
+      return (uint8_t)a[i] < (uint8_t)b[i] ? -1 : 1;
+  }
+#else
   lj_assertX(len > 0, "fast string compare with zero length");
   lj_assertX((((uintptr_t)a+len-1) & (LJ_PAGESIZE-1)) <= LJ_PAGESIZE-4,
 	     "fast string compare crossing page boundary");
@@ -68,6 +94,7 @@ static LJ_AINLINE int str_fastcmp(const char *a, const char *b, MSize len)
     }
     i += 4;
   } while (i < len);
+#endif
   return 0;
 }
 
@@ -83,7 +110,11 @@ const char *lj_str_find(const char *s, const char *p, MSize slen, MSize plen)
       while (slen) {
 	const char *q = (const char *)memchr(s, c, slen);
 	if (!q) break;
+#if LUAJIT_USE_VALGRIND
+  if (lj_str_cmp_no_opt(q+1, p, plen) == 0) return q;
+#else
 	if (memcmp(q+1, p, plen) == 0) return q;
+#endif
 	q++; slen -= (MSize)(q-s); s = q;
       }
     }
@@ -232,8 +263,13 @@ GCstr *lj_str_new(lua_State *L, const char *str, size_t lenx)
   } else {  /* Slow path: end of string is too close to a page boundary. */
     while (o != NULL) {
       GCstr *sx = gco2str(o);
+#if LUAJIT_USE_VALGRIND
+      if (sx->hash == h && sx->len == len && inc_collision_hard() &&
+                      lj_str_cmp_no_opt(str, strdata(sx), len) == 0) {
+#else
       if (sx->hash == h && sx->len == len && inc_collision_hard() &&
                       memcmp(str, strdata(sx), len) == 0) {
+#endif
 	/* Resurrect if dead. Can only happen with fixstring() (keywords). */
 	if (isdead(g, o)) flipwhite(o);
 	g->strhash_hit++;
@@ -277,7 +313,11 @@ GCstr *lj_str_new(lua_State *L, const char *str, size_t lenx)
 	} else {  /* Slow path: end of string is too close to a page boundary. */
 	  while (o != NULL) {
 	    GCstr *sx = gco2str(o);
+#if LUAJIT_USE_VALGRIND
+	    if (sx->hash == fh && sx->len == len && lj_str_cmp_no_opt(str, strdata(sx), len) == 0) {
+#else
 	    if (sx->hash == fh && sx->len == len && memcmp(str, strdata(sx), len) == 0) {
+#endif
 	      /* Resurrect if dead. Can only happen with fixstring() (keywords). */
 	      if (isdead(g, o)) flipwhite(o);
 	      g->strhash_hit++;
@@ -323,5 +363,4 @@ void LJ_FASTCALL lj_str_free(global_State *g, GCstr *s)
 {
   g->strnum--;
   lj_mem_free(g, s, sizestring(s));
-}
-
+}
\ No newline at end of file
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit 1/2] c: disable strcmp optimization in Valgrind build

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
Please consider my comment below.

On 26.06.24, mandesero@gmail.com wrote:
> From: mandesero <mandesero@gmail.com>
> 
> ---
>  src/lj_str.c | 43 +++++++++++++++++++++++++++++++++++++++++--

AFAICS, the <src/lj.supp> contains Valgrind suppressions for
`lj_str_cmp()` and `str_fastcmp()`. The upstream politics until
ff34b48ddd6f2b3bdd26d6088662a214ba6b0288 ("Redesign and harden string
interning.") [1] is to use this file for all suppressions (see the
corresponding comments in the commit). So I suggest dropping this patch
and introducing testing with suppressions in the CI in another patch (as
it is done). Are there any objections to this?

[1]: https://github.com/LuaJIT/LuaJIT/commit/ff34b48ddd6f2b3bdd26d6088662a214ba6b0288

>  1 file changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lj_str.c b/src/lj_str.c
> index 321e8c4f..00cced03 100644
> --- a/src/lj_str.c
> +++ b/src/lj_str.c

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 1/2] c: disable strcmp optimization in Valgrind build

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4375 bytes --]

Hi, Maxim,

thanks for the patch!

The patch below is quite similar to the patch in OpenResty [1].

Have you used it?

1. 
https://github.com/openresty/luajit2/commit/6315a752274f3a4db6827b64788173f40733204e

On 26.06.2024 15:27, mandesero--- via Tarantool-patches wrote:
> From: mandesero<mandesero@gmail.com>
>
> ---
>   src/lj_str.c | 43 +++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/src/lj_str.c b/src/lj_str.c
> index 321e8c4f..00cced03 100644
> --- a/src/lj_str.c
> +++ b/src/lj_str.c
> @@ -26,10 +26,29 @@ static LJ_AINLINE int str_fastcmp(const char *a, const char *b, MSize len)
>   
>   /* -- String helpers ------------------------------------------------------ */
>   
> +#if LUAJIT_USE_VALGRIND
> +int lj_str_cmp_no_opt(const char *a, const char *b, MSize len)
> +{
> +  for (MSize i = 0; i < len; i++)
> +    if (a[i] != b[i])
> +      return 1;
> +  return 0;
> +}
> +#endif
> +
>   /* Ordered compare of strings. Assumes string data is 4-byte aligned. */
>   int32_t LJ_FASTCALL lj_str_cmp(GCstr *a, GCstr *b)
>   {
>     MSize i, n = a->len > b->len ? b->len : a->len;
> +#if LUAJIT_USE_VALGRIND
> +  const uint8_t *sa = (const uint8_t *)strdata(a);
> +  const uint8_t *sb = (const uint8_t *)strdata(b);
> +
> +  for (i = 0; i < n; i++) {
> +    if (sa[i] != sb[i])
> +      return (uint8_t)sa[i] < (uint8_t)sb[i] ? -1 : 1;
> +  }
> +#else
>     for (i = 0; i < n; i += 4) {
>       /* Note: innocuous access up to end of string + 3. */
>       uint32_t va = *(const uint32_t *)(strdata(a)+i);
> @@ -46,6 +65,7 @@ int32_t LJ_FASTCALL lj_str_cmp(GCstr *a, GCstr *b)
>         return va < vb ? -1 : 1;
>       }
>     }
> +#endif
>     return (int32_t)(a->len - b->len);
>   }
>   
> @@ -53,6 +73,12 @@ int32_t LJ_FASTCALL lj_str_cmp(GCstr *a, GCstr *b)
>   static LJ_AINLINE int str_fastcmp(const char *a, const char *b, MSize len)
>   {
>     MSize i = 0;
> +#if LUAJIT_USE_VALGRIND
> +  for (i = 0; i < len; i++) {
> +    if (a[i] != b[i])
> +      return (uint8_t)a[i] < (uint8_t)b[i] ? -1 : 1;
> +  }
> +#else
>     lj_assertX(len > 0, "fast string compare with zero length");
>     lj_assertX((((uintptr_t)a+len-1) & (LJ_PAGESIZE-1)) <= LJ_PAGESIZE-4,
>   	     "fast string compare crossing page boundary");
> @@ -68,6 +94,7 @@ static LJ_AINLINE int str_fastcmp(const char *a, const char *b, MSize len)
>       }
>       i += 4;
>     } while (i < len);
> +#endif
>     return 0;
>   }
>   
> @@ -83,7 +110,11 @@ const char *lj_str_find(const char *s, const char *p, MSize slen, MSize plen)
>         while (slen) {
>   	const char *q = (const char *)memchr(s, c, slen);
>   	if (!q) break;
> +#if LUAJIT_USE_VALGRIND
> +  if (lj_str_cmp_no_opt(q+1, p, plen) == 0) return q;
> +#else
>   	if (memcmp(q+1, p, plen) == 0) return q;
> +#endif
>   	q++; slen -= (MSize)(q-s); s = q;
>         }
>       }
> @@ -232,8 +263,13 @@ GCstr *lj_str_new(lua_State *L, const char *str, size_t lenx)
>     } else {  /* Slow path: end of string is too close to a page boundary. */
>       while (o != NULL) {
>         GCstr *sx = gco2str(o);
> +#if LUAJIT_USE_VALGRIND
> +      if (sx->hash == h && sx->len == len && inc_collision_hard() &&
> +                      lj_str_cmp_no_opt(str, strdata(sx), len) == 0) {
> +#else
>         if (sx->hash == h && sx->len == len && inc_collision_hard() &&
>                         memcmp(str, strdata(sx), len) == 0) {
> +#endif
>   	/* Resurrect if dead. Can only happen with fixstring() (keywords). */
>   	if (isdead(g, o)) flipwhite(o);
>   	g->strhash_hit++;
> @@ -277,7 +313,11 @@ GCstr *lj_str_new(lua_State *L, const char *str, size_t lenx)
>   	} else {  /* Slow path: end of string is too close to a page boundary. */
>   	  while (o != NULL) {
>   	    GCstr *sx = gco2str(o);
> +#if LUAJIT_USE_VALGRIND
> +	    if (sx->hash == fh && sx->len == len && lj_str_cmp_no_opt(str, strdata(sx), len) == 0) {
> +#else
>   	    if (sx->hash == fh && sx->len == len && memcmp(str, strdata(sx), len) == 0) {
> +#endif
>   	      /* Resurrect if dead. Can only happen with fixstring() (keywords). */
>   	      if (isdead(g, o)) flipwhite(o);
>   	      g->strhash_hit++;
> @@ -323,5 +363,4 @@ void LJ_FASTCALL lj_str_free(global_State *g, GCstr *s)
>   {
>     g->strnum--;
>     lj_mem_free(g, s, sizestring(s));
> -}
> -
> +}
> \ No newline at end of file

[-- Attachment #2: Type: text/html, Size: 4975 bytes --]

[Tarantool-patches] [PATCH luajit 2/2] cmake: running tests under Valgrind, disable tests that failed under Valgrind

[mandesero--- via Tarantool-patches]

From: mandesero <mandesero@gmail.com>

---
 .github/actions/setup-sanitizers/action.yml |  2 +-
 .github/workflows/sanitizers-testing.yml    | 53 +++++++++++++++++++++
 src/lj_no_str_opt.supp                      | 16 +++++++
 test/CMakeLists.txt                         |  6 ++-
 test/tarantool-tests/CMakeLists.txt         | 17 +++++++
 5 files changed, 92 insertions(+), 2 deletions(-)
 create mode 100644 src/lj_no_str_opt.supp

diff --git a/.github/actions/setup-sanitizers/action.yml b/.github/actions/setup-sanitizers/action.yml
index 8642d553..53f95075 100644
--- a/.github/actions/setup-sanitizers/action.yml
+++ b/.github/actions/setup-sanitizers/action.yml
@@ -20,7 +20,7 @@ runs:
     - name: Install build and test dependencies
       run: |
         apt -y update
-        apt -y install ${CC_NAME} libstdc++-10-dev cmake ninja-build make perl
+        apt -y install ${CC_NAME} libstdc++-10-dev cmake ninja-build make perl valgrind
       shell: bash
       env:
         CC_NAME: ${{ inputs.cc_name }}
diff --git a/.github/workflows/sanitizers-testing.yml b/.github/workflows/sanitizers-testing.yml
index 154ebe40..85ccf580 100644
--- a/.github/workflows/sanitizers-testing.yml
+++ b/.github/workflows/sanitizers-testing.yml
@@ -93,3 +93,56 @@ jobs:
           "
         run: cmake --build . --parallel --target LuaJIT-test
         working-directory: ${{ env.BUILDDIR }}
+
+
+  test-valgrind:
+    strategy:
+      fail-fast: false
+      matrix:
+        # XXX: Let's start with only Linux/x86_64
+        BUILDTYPE: [Debug, Release]
+        CC: [gcc-10, clang-11]
+        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 Valgrind (Linux/x86_64)
+      ${{ matrix.BUILDTYPE }}
+      CC:${{ matrix.CC }}
+      GC64:ON SYSMALLOC:ON
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+          submodules: recursive
+      - name: setup Linux for sanitizers
+        uses: ./.github/actions/setup-sanitizers
+        with:
+          cc_name: ${{ matrix.CC }}
+      - name: configure
+        # XXX: LuaJIT configuration requires a couple of tweaks:
+        # LUAJIT_USE_SYSMALLOC=ON: Valgrind requires the use of
+        #   a system-provided memory allocator (realloc) instead
+        #   of the built-in memory allocator.
+        #   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.
+        run: >
+          cmake -S . -B ${{ env.BUILDDIR }}
+          -G Ninja
+          ${{ matrix.CMAKEFLAGS }}
+          -DLUAJIT_USE_VALGRIND=ON
+          -DLUAJIT_USE_SYSMALLOC=ON
+          -DLUAJIT_ENABLE_GC64=ON
+      - name: build
+        run: cmake --build . --parallel
+        working-directory: ${{ env.BUILDDIR }}
+      - name: test
+        run: cmake --build . --parallel --target LuaJIT-test
+        working-directory: ${{ env.BUILDDIR }}
\ No newline at end of file
diff --git a/src/lj_no_str_opt.supp b/src/lj_no_str_opt.supp
new file mode 100644
index 00000000..1ec470f8
--- /dev/null
+++ b/src/lj_no_str_opt.supp
@@ -0,0 +1,16 @@
+# Valgrind suppression file for LuaJIT 2.0.
+{
+   Optimized string compare
+   Memcheck:Addr4
+   fun:lj_str_new
+}
+{
+   Optimized string compare
+   Memcheck:Addr1
+   fun:lj_str_new
+}
+{
+   Optimized string compare
+   Memcheck:Cond
+   fun:lj_str_new
+}
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 19726f5a..fc2791f6 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -71,7 +71,11 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS
   ${PROJECT_NAME}-codespell
 )
 
-set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
+if(LUAJIT_USE_VALGRIND)
+  set(LUAJIT_TEST_COMMAND "valgrind --suppressions=${CMAKE_SOURCE_DIR}/src/lj_no_str_opt.supp ${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
+else()
+  set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
+endif()
 separate_arguments(LUAJIT_TEST_COMMAND)
 
 set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index d7c96078..ac09e993 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -121,9 +121,26 @@ add_test_suite_target(tarantool-tests
   DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-libs
 )
 
+# Tests are disabled as they fail with Valgrind enabled due to SIGPROF
+# signals from the profiler or exceeding the maximum test duration time.
+list(APPEND valgrind_excluded_tests
+  "gh-5688-tool-cli-flag.test.lua"
+  "gh-5813-resolving-of-c-symbols.test.lua"
+  "gh-7264-add-proto-trace-sysprof-default.test.lua"
+  "gh-7745-oom-on-trace.test.lua"
+  "lj-1034-tabov-error-frame.test.lua"
+  "lj-512-profiler-hook-finalizers.test.lua"
+  "lj-726-profile-flush-close.test.lua"
+  "misclib-sysprof-lapi.test.lua"
+)
+
 file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}")
 foreach(test_path ${tests})
   get_filename_component(test_name ${test_path} NAME)
+  list(FIND valgrind_excluded_tests ${test_name} test_index)
+  if(LUAJIT_USE_VALGRIND AND NOT test_index EQUAL -1)
+    continue()
+  endif()
   set(test_title "test/${TEST_SUITE_NAME}/${test_name}")
   add_test(NAME ${test_title}
     COMMAND ${LUAJIT_TEST_COMMAND} ${test_path}
-- 
2.34.1