[Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option
Sergey Bronnikov
sergeyb at tarantool.org
Thu Jun 13 18:13:11 MSK 2024
Hi, Sergey
thanks for the fixes and answers!
LGTM, anyway, please take a look at my answers below.
On 13.06.2024 13:56, Sergey Kaplun wrote:
> 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.
Thanks!
<snipped>
>> 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.
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.
I would replace "not interested" to smthing like "maintainer not
interested".
Feel free to ignore.
>
>>> + )
>>> + 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20240613/54f68c5e/attachment.htm>
More information about the Tarantool-patches
mailing list