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