<!DOCTYPE html>
<html data-lt-installed="true">
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body style="padding-bottom: 1px;">
    <p>Hi, Sergey</p>
    <p>thanks for the fixes and answers!</p>
    <p>LGTM, anyway, please take a look at my answers below.<br>
    </p>
    <div class="moz-cite-prefix">On 13.06.2024 13:56, Sergey Kaplun
      wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:ZmrQdA2hvxv2Cgum@root">
      <pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the review!
Please considered my answers below.

On 07.06.24, Sergey Bronnikov wrote:
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">Sergey,

thanks for the patch! Please see my comments below.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Fixed your comments, see the iterative patch below.
The branch is force-pushed.</pre>
    </blockquote>
    Thanks!<br>
    <blockquote type="cite" cite="mid:ZmrQdA2hvxv2Cgum@root">
      <pre class="moz-quote-pre" wrap="">
</pre>
    </blockquote>
    <snipped><br>
    <blockquote type="cite" cite="mid:ZmrQdA2hvxv2Cgum@root">
      <pre class="moz-quote-pre" wrap="">
</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">
On 15.05.2024 15:32, Sergey Kaplun wrote:
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">This patch adds Undefined Behaviour Sanitizer [1] support. It enables
all checks except several that are not useful for LuaJIT. Also, it
instruments all known issues to be fixed in future patches (except
`kfold_intop()` since cdata arithmetic relies on integer overflow).

[1]:<a class="moz-txt-link-freetext" href="https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html">https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html</a>

Resolves tarantool/tarantool#8473
---
  CMakeLists.txt             | 45 ++++++++++++++++++++++++++++++++++++++
  cmake/SetDynASMFlags.cmake | 11 ++++++++++
  src/lj_carith.c            |  5 +++++
  src/lj_opt_fold.c          |  5 +++++
  src/lj_parse.c             |  5 +++++
  src/lj_snap.c              |  7 ++++++
  src/lj_strfmt.c            |  5 +++++
  7 files changed, 83 insertions(+)
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
patch in mail is outdated, so I'll copypaste missed part:
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Yes, it is mentioned in this subthread [1].

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">

diff --git a/src/lj_buf.h b/src/lj_buf.h
index a4051694..aaecc9f8 100644
--- a/src/lj_buf.h
+++ b/src/lj_buf.h
@@ -70,6 +70,13 @@ LJ_FUNC SBuf *lj_buf_putmem(SBuf *sb, const void *q, 
MSize len);
  LJ_FUNC SBuf * LJ_FASTCALL lj_buf_putchar(SBuf *sb, int c);
  LJ_FUNC SBuf * LJ_FASTCALL lj_buf_putstr(SBuf *sb, GCstr *s);

+#if LUAJIT_USE_UBSAN
+/* The `NULL` argument with the zero length, like in the case:
+** | luajit -e 'error("x", 3)'
+*/
+static LJ_AINLINE char *lj_buf_wmem(char *p, const void *q, MSize len)
+  __attribute__((no_sanitize("nonnull-attribute")));
+#endif
  static LJ_AINLINE char *lj_buf_wmem(char *p, const void *q, MSize len)
  {
    return (char *)memcpy(p, q, len) + len;


With this reverted patch tests passed. Do we really need this patch?
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Should I add the corresponding test mentioned in [1]?

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">

</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2355ce17..edf2012f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -300,6 +300,51 @@ if(LUAJIT_USE_ASAN)
    )
  endif()
  
+option(LUAJIT_USE_UBSAN "Build LuaJIT with UndefinedBehaviorSanitizer" OFF)
+if(LUAJIT_USE_UBSAN)
+  # Use all recommendations from the UndefinedBehaviorSanitizer
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
probably you mean "checks" [1] and not "recommendations"
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Fixed, thanks.</pre>
    </blockquote>
    <p>Thanks!</p>
    <p><br>
    </p>
    <blockquote type="cite" cite="mid:ZmrQdA2hvxv2Cgum@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">

1. <a class="moz-txt-link-freetext" href="https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks">https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks</a>

</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">+  # documentation:
+  #<a class="moz-txt-link-freetext" href="https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html">https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html</a>.
+  string(JOIN "," UBSAN_IGNORE_OPTIONS
+    # Misaligned pseudo-pointers are used to determine internal
+    # variable names inside the `for` cycle.
+    alignment
+    # Not interested in float cast overflow errors.
+    float-cast-overflow
+    # NULL checking is disabled because this is not a UB and
+    # raises lots of false-positive fails.
+    null
+    # Not interested in checking arithmetic with NULL.
+    pointer-overflow
+    # Shifts of negative numbers are widely used in parsing ULEB,
+    # cdata arithmetic, vmevent hash calculation, etc.
+    shift-base
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
Will we report issues produced by these checks to upstream?
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
These particular checks -- no, since they are not so interesting for us,
and most probably may be considered by Mike as "white noise".

For others -- yes.
I've already reported the related problem with the patch [3].

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">
Decision "not interested" confuses.
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
I've given the rationale for float-cast-overflow [2].
Pointer overflow is not interesting for us since it is widely used in
LuaJIT, in particular in <lj_alloc.c>. So, we may avoid warnings in
`NULL - ptr` arithmetics.</pre>
    </blockquote>
    <p>I would replace "not interested" to smthing like "maintainer not
      interested".</p>
    <p>Feel free to ignore.<br>
    </p>
    <blockquote type="cite" cite="mid:ZmrQdA2hvxv2Cgum@root">
      <pre class="moz-quote-pre" wrap="">

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">+  )
+  if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">
please add a link to GCC documentation

<a class="moz-txt-link-freetext" href="https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003dundefined">https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003dundefined</a>
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Added, thanks.

</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">
</pre>
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">+    string(JOIN "," UBSAN_IGNORE_OPTIONS
+      ${UBSAN_IGNORE_OPTIONS}
+      # Not interested in function type mismatch errors.
+      function
+    )
+  endif()
+  AppendFlags(CMAKE_C_FLAGS
+    # Enable hints for UndefinedBehaviorSanitizer.
+    -DLUAJIT_USE_UBSAN
+    # XXX: To get nicer stack traces in error messages.
+    -fno-omit-frame-pointer
+    # Enable UndefinedBehaviorSanitizer support.
+    # This flag enables all supported options (the documentation
+    # on cite is not correct about that moment, unfortunately)
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">typo: cite -> site
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Fixed, thanks.

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">+    # except float-divide-by-zero. Floating point division by zero
+    # behaviour is defined without -ffast-math and uses the
+    # IEEE 754 standard on which all NaN tagging is based.
+    -fsanitize=undefined
+    -fno-sanitize=${UBSAN_IGNORE_OPTIONS}
+    # Print a verbose error report and exit the program.
+    -fno-sanitize-recover=undefined
+  )
+endif()
+
  # Enable code coverage support.
  option(LUAJIT_ENABLE_COVERAGE "Enable code coverage support (gcovr)" OFF)
  if(LUAJIT_ENABLE_COVERAGE)
diff --git a/cmake/SetDynASMFlags.cmake b/cmake/SetDynASMFlags.cmake
index 7eead6e9..ae3c75b1 100644
--- a/cmake/SetDynASMFlags.cmake
+++ b/cmake/SetDynASMFlags.cmake
@@ -136,5 +136,16 @@ if(NOT CMAKE_SYSTEM_NAME STREQUAL ${CMAKE_HOST_SYSTEM_NAME})
    endif()
  endif()
  
+if(LUAJIT_USE_UBSAN)
+  # XXX: Skip checks for now to avoid build failures due to
+  # sanitizer errors.
+  # Need to backprot commits that fix the following issues first:
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">typo: backprot -> backport
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Fixed, thanks!

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">+  #<a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/pull/969">https://github.com/LuaJIT/LuaJIT/pull/969</a>,
+  #<a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/pull/970">https://github.com/LuaJIT/LuaJIT/pull/970</a>,
+  #<a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1041">https://github.com/LuaJIT/LuaJIT/issues/1041</a>,
+  #<a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/pull/1044">https://github.com/LuaJIT/LuaJIT/pull/1044</a>.
+  AppendFlags(HOST_C_FLAGS -fno-sanitize=undefined)
+endif()
+
  unset(LUAJIT_ARCH)
  unset(TESTARCH)
diff --git a/src/lj_carith.c b/src/lj_carith.c
</pre>
        </blockquote>
        <pre class="moz-quote-pre" wrap="">With this reverted patch tests passed. Do we really need this patch?
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Yes, since cdata arithmetics depends on overflows of integers. So we
should ignore all warnings here.

</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre class="moz-quote-pre" wrap="">index 4e1d450a..1d9d6fe1 100644
--- a/src/lj_carith.c
+++ b/src/lj_carith.c
@@ -159,6 +159,11 @@ static int carith_ptr(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
  }
  
  /* 64 bit integer arithmetic. */
+#if LUAJIT_USE_UBSAN
+/* Seehttps://github.com/LuaJIT/LuaJIT/issues/928. */
+static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
+  __attribute__((no_sanitize("signed-integer-overflow")));
+#endif
  static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
  {
    if (ctype_isnum(ca->ct[0]->info) && ca->ct[0]->size <= 8 &&
</pre>
        </blockquote>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
<snipped>

[1]: <a class="moz-txt-link-freetext" href="https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029185.html">https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029185.html</a>
[2]: <a class="moz-txt-link-freetext" href="https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029195.html">https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029195.html</a>
[3]: <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1193">https://github.com/LuaJIT/LuaJIT/issues/1193</a>

</pre>
    </blockquote>
  </body>
  <lt-container></lt-container>
</html>