Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the review!
Please considered my answers below.

On 07.06.24, Sergey Bronnikov wrote:
> Sergey,
> 
> thanks for the patch! Please see my comments below.

Fixed your comments, see the iterative patch below.
The branch is force-pushed.

===================================================================
diff --git a/CMakeLists.txt b/CMakeLists.txt
index e00b1536..76e0c067 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -302,7 +302,7 @@ endif()
 
 option(LUAJIT_USE_UBSAN "Build LuaJIT with UndefinedBehaviorSanitizer" OFF)
 if(LUAJIT_USE_UBSAN)
-  # Use all recommendations from the UndefinedBehaviorSanitizer
+  # Use all needed checks from the UndefinedBehaviorSanitizer
   # documentation:
   # https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html.
   string(JOIN "," UBSAN_IGNORE_OPTIONS
@@ -322,7 +322,8 @@ if(LUAJIT_USE_UBSAN)
     # cdata arithmetic, vmevent hash calculation, etc.
     shift-base
   )
-  # GCC has no "function" UB check.
+  # GCC has no "function" UB check. See details here:
+  # https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003dundefined
   if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
     string(JOIN "," UBSAN_IGNORE_OPTIONS
       ${UBSAN_IGNORE_OPTIONS}
@@ -337,7 +338,7 @@ if(LUAJIT_USE_UBSAN)
     -fno-omit-frame-pointer
     # Enable UndefinedBehaviorSanitizer support.
     # This flag enables all supported options (the documentation
-    # on cite is not correct about that moment, unfortunately)
+    # on site is not correct about that moment, unfortunately)
     # 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.
diff --git a/cmake/SetDynASMFlags.cmake b/cmake/SetDynASMFlags.cmake
index ae3c75b1..b400cf57 100644
--- a/cmake/SetDynASMFlags.cmake
+++ b/cmake/SetDynASMFlags.cmake
@@ -139,7 +139,7 @@ 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:
+  # Need to backport commits that fix the following issues first:
   # https://github.com/LuaJIT/LuaJIT/pull/969,
   # https://github.com/LuaJIT/LuaJIT/pull/970,
   # https://github.com/LuaJIT/LuaJIT/issues/1041,
===================================================================

> 
> On 15.05.2024 15:32, Sergey Kaplun wrote:
> > 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]:https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
> >
> > 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(+)
> 
> patch in mail is outdated, so I'll copypaste missed part:

Yes, it is mentioned in this subthread [1].

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

Should I add the corresponding test mentioned in [1]?

> 
> 
> >
> > 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
> 
> probably you mean "checks" [1] and not "recommendations"

Fixed, thanks.

> 
> 
> 1. https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks
> 
> > +  # documentation:
> > +  #https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html.
> > +  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
> 
> Will we report issues produced by these checks to upstream?

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].

> 
> Decision "not interested" confuses.

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.

> 
> > +  )
> > +  if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
> 
> please add a link to GCC documentation
> 
> https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003dundefined

Added, thanks.

> 
> > +    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)
> typo: cite -> site

Fixed, thanks.

> > +    # 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:
> typo: backprot -> backport

Fixed, thanks!

> > +  #https://github.com/LuaJIT/LuaJIT/pull/969,
> > +  #https://github.com/LuaJIT/LuaJIT/pull/970,
> > +  #https://github.com/LuaJIT/LuaJIT/issues/1041,
> > +  #https://github.com/LuaJIT/LuaJIT/pull/1044.
> > +  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
> With this reverted patch tests passed. Do we really need this patch?

Yes, since cdata arithmetics depends on overflows of integers. So we
should ignore all warnings here.

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

<snipped>

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029185.html
[2]: https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029195.html
[3]: https://github.com/LuaJIT/LuaJIT/issues/1193

-- 
Best regards,
Sergey Kaplun