From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 78E2BBA2F30; Thu, 13 Jun 2024 18:13:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 78E2BBA2F30 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1718291594; bh=CahJ6LcI+yBXwrt5o1i899R/ENrdhIHGfE86DWdNthQ=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=hrTRH+Yaq/3selpX9lH4tAx18Va2+gbKVATqpis4v3Ava1XmOdI3Oqk3HwXNalZLY i7bcqGJfO2yKClMUIGSQlyDt6qsxNV3hnJNFed25bj7Glmtrm+ZagC718BRXWZNR2H aMlDWK0w3UeHIXakuGrEzLf/udh/UG+V3xECRx5I= Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [95.163.41.82]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CB991BA2F2D for ; Thu, 13 Jun 2024 18:13:12 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org CB991BA2F2D Received: by smtp44.i.mail.ru with esmtpa (envelope-from ) id 1sHm8V-0000000CtFa-3W1q; Thu, 13 Jun 2024 18:13:12 +0300 Content-Type: multipart/alternative; boundary="------------YwHmEn0Zbj33Bu3X8ZsssACo" Message-ID: <80357e90-fa66-4f12-b72e-468c8ea0278d@tarantool.org> Date: Thu, 13 Jun 2024 18:13:11 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun References: <6f8a08e1823bfceebb4057207ee2f2bdb7d2d47c.1715776117.git.skaplun@tarantool.org> <3605e667-a4e6-4ee1-abd2-412e81d76c89@tarantool.org> In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9AC8CA0B4439200FA19AE3EA0235AE0E7243ECEBF822142BF00894C459B0CD1B9130BC33769005E5F0771F2AEDB3B3FF74B2A558357548C510539033E3746DDD8D64C754CAC7C657B X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7922FDBD9EBA3C5B4B287FD4696A6DC2FA8DF7F3B2552694A4E2F5AFA99E116B42401471946AA11AF7680F9384605B903A8883CB087864CAC8F08D7030A58E5AD1A62830130A00468AEEEE3FBA3A834EE7353EFBB553375660C7ECA9C91037E2E3D21955FAFCD93415437B37623EA6ABA86EC207AD144F717389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0A3E989B1926288338941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B652D31B9D28593E51CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C224925ECCA29681764E776E601842F6C81A12EF20D2F80756B5FB606B96278B59C4276E601842F6C81A127C277FBC8AE2E8B803160D0BC219E2D3AA81AA40904B5D99C9F4D5AE37F343AD1F44FA8B9022EA23BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735F1C9CF18C8EB2269C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A57144BA63FC76F60C5002B1117B3ED69666B8EBDFC316D1FA47A99E6294EE8661823CB91A9FED034534781492E4B8EEADB1D70E2111C441FFBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF16741E977628277575FAFC23104F620C45CDF6FB889E2F3EE655197ED5E1D20E80A302719471681B04A26A3878DC1330C82EEF256322BD156B19083EC5CD2465BF777C26DBC9435F5F4332CA8FE04980913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojP/c/PTD82Anngc/WWjgm1A== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D6140130BC33769005E5F0771F2AEDB3B3FF70CEC2EA2E025A9780152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/2] build: introduce LUAJIT_USE_UBSAN option X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------YwHmEn0Zbj33Bu3X8ZsssACo Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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! >> 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 . 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 && > > > [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 > --------------YwHmEn0Zbj33Bu3X8ZsssACo Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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

--------------YwHmEn0Zbj33Bu3X8ZsssACo--