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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On Wed, May 15, 2024 at 03:32:00PM UTC, 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(+)
>
> 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
> +  # 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
Why we are not interested in them though?
> +    # 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
> +  )
> +  if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU")
> +    string(JOIN "," UBSAN_IGNORE_OPTIONS
> +      ${UBSAN_IGNORE_OPTIONS}
> +      # Not interested in function type mismatch errors.
> +      function
> +    )
> +  endif()
Please drop a comment explaining why this additional configuration
is needed.
> +  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)
> +    # 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()

The whole chunk above is a bit too large to include into the root
CMakeLists.txt, so I propose to move it into a separate CMake module.
> +
<snipped>