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 DF572B9B23A; Mon, 27 May 2024 15:32:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DF572B9B23A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1716813178; bh=IujgPZYwBPQV1v8lFNKvCCMDrZMOFZ+mVmcIX3m7XFE=; 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=rHZihyhwNrN+Stml0/xeqamtA1SlISczboLT06Nq3MApYGAdA5lYZplyggbw/Jlkc oYhahNbJy2vwnidoIXFsFLG0syzRGKQUKpbyjHk8ik7hq3LaSk+yLIuhB0CgUnzI36 m2t04ijGYE0OxHXLyGxjvTg6SJk+7+DN7EOLm29c= Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [95.163.41.80]) (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 BA00FB9B200 for ; Mon, 27 May 2024 15:32:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BA00FB9B200 Received: by smtp39.i.mail.ru with esmtpa (envelope-from ) id 1sBZX4-0000000Ay8W-2OOi; Mon, 27 May 2024 15:32:55 +0300 Date: Mon, 27 May 2024 15:28:41 +0300 To: Maxim Kokryashkin Message-ID: References: <6f8a08e1823bfceebb4057207ee2f2bdb7d2d47c.1715776117.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD94C4E2F1F16A96B38DE0848B9E8C07778C5FBA1DD76A3D6671313CFAB8367EF908E2BE116634AD74DEF8D8F3180C1D14288A9B2395F56A39F627BF1DCFB0016AA94A270A90C8DA216E1C7414A75FA687D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7798B95EC47D21699EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063792A46A0C27F4DAA78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D82217FC6AF07F468F59DC0B504338385B52A83EECC1477BC1CC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC81D471462564A2E19F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CCE135D2742255B35AD7EC71F1DB884274AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C31580D188F428539ABA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CFE478A468B35FE7671DD303D21008E298D5E8D9A59859A8B65FF0BFC5AEE34BE675ECD9A6C639B01B78DA827A17800CE7D699F3A2029486C7731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A55DAE5ABD6EB16BC05002B1117B3ED696A5A18A8609DB7598B2920F75BA9A967F823CB91A9FED034534781492E4B8EEAD0AA277257C6A5E3DBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34C974B02B4EA30DFB8E2F67EE2CD1A504005AEAB371791A6DEE5F71C45B9DF2F22A0360FE720801A81D7E09C32AA3244C279CA3CA9F8EC1DFEBBA027F22CEDE66B627431A2D628EA2EA455F16B58544A2557BDE0DD54B3590A5AE236DF995FB59829709634694AABAED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj0imwrCqrDNAUgK5KXztYuw== X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B599602875B4FA57134C591814E25D11F9FB279EFC14AA9022DB7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim! Thanks for the review! Please consider my answers below. On 27.05.24, Maxim Kokryashkin wrote: > 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? Most warnings are referenced by `lj_num2int()`, which just converts the `double` to `int`. All such checks are covered by the corresponding check that the resulted value of the `int` cast back to `double` remains the same number. I've added the following comment: =================================================================== diff --git a/CMakeLists.txt b/CMakeLists.txt index edf2012f..5eeea783 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -309,7 +309,9 @@ if(LUAJIT_USE_UBSAN) # Misaligned pseudo-pointers are used to determine internal # variable names inside the `for` cycle. alignment - # Not interested in float cast overflow errors. + # Not interested in float cast overflow errors. These + # overflows are handled by special checks after + # `lj_num2int()`, etc. float-cast-overflow # NULL checking is disabled because this is not a UB and # raises lots of false-positive fails. =================================================================== > > + # 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. Added. =================================================================== diff --git a/CMakeLists.txt b/CMakeLists.txt index edf2012f..5eeea783 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -320,6 +322,7 @@ if(LUAJIT_USE_UBSAN) # cdata arithmetic, vmevent hash calculation, etc. shift-base ) + # GCC has no "function" UB check. if(NOT CMAKE_C_COMPILER_ID STREQUAL "GNU") string(JOIN "," UBSAN_IGNORE_OPTIONS ${UBSAN_IGNORE_OPTIONS} =================================================================== > > + 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. It has a size similar to the ASAN blob (44 lines, most of which are comments, against 25 lines), so ignoring. > > + > Also, added the additional instrumentation for `lj_buf_wmem()` as a result of this discussion [1]: =================================================================== diff --git a/src/lj_buf.h b/src/lj_buf.h index a4051694..de7bee06 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; =================================================================== [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2024-May/029192.html -- Best regards, Sergey Kaplun