[Tarantool-patches] [PATCH 0/2] ASAN build

[Tarantool-patches] [PATCH 0/2] ASAN build

[Vladislav Shpilevoy]

The build was broken because sql static library couldn't find some
box symbols, when UB sanitizer was enabled.

Appeared, that the whole sql library was broken, because there was
a cyclic dependency between box and sql libraries.

The patch merged sql and box libraries.

After their merge appeared that some UB sanitations 'woken up' and
started failing, about nullability attributes and usage of NULL
pointers. All of them were false-positive so the second patch
disabled them.

After this patchset the build works, but the tests don't pass
because of this:
https://github.com/tarantool/tarantool/issues/5078.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5067-asan-build
Issue: https://github.com/tarantool/tarantool/issues/5067
Issue: https://github.com/tarantool/tarantool/issues/4609

Vladislav Shpilevoy (2):
  sql: don't build sql as a separate library
  cmake: split UB sanitations into separate flags.

 cmake/compiler.cmake       |  44 +++++++++++++--
 src/CMakeLists.txt         |   2 +-
 src/box/CMakeLists.txt     | 101 ++++++++++++++++++++++++++++++++--
 src/box/sql/CMakeLists.txt | 109 -------------------------------------
 4 files changed, 138 insertions(+), 118 deletions(-)
 delete mode 100644 src/box/sql/CMakeLists.txt

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 1/2] sql: don't build sql as a separate library

[Vladislav Shpilevoy]

SQL heavily depends on box, and box on SQL. So they can't be
separate libraries. The build started failing with undefined box
symbols in SQL, when code of the latter has slightly changed in
one of the recent commits.

The build failed only with UB sanitizer enabled, but
'VERBOSE=1 make' showed that both with UB and without UB the build
command was the same (not counting -fsanitize flags). So the
sanitizer has nothing to do with it.

The patch makes SQL sources being built as a part of box library.

Closes #5067
---
 src/CMakeLists.txt         |   2 +-
 src/box/CMakeLists.txt     | 101 ++++++++++++++++++++++++++++++++--
 src/box/sql/CMakeLists.txt | 109 -------------------------------------
 3 files changed, 98 insertions(+), 114 deletions(-)
 delete mode 100644 src/box/sql/CMakeLists.txt

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 7099e9bef..68d69eded 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -266,7 +266,7 @@ add_executable(
     ${LIBUTIL_FREEBSD_SRC}/flopen.c
     ${LIBUTIL_FREEBSD_SRC}/pidfile.c)
 
-add_dependencies(tarantool build_bundled_libs sql)
+add_dependencies(tarantool build_bundled_libs)
 target_link_libraries(tarantool box ${common_libraries})
 
 if (TARGET_OS_FREEBSD AND NOT TARGET_OS_DEBIAN_FREEBSD)
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 230e7427d..37a3c3b50 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -1,12 +1,11 @@
 file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/src/box/lua)
+file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/src/box/sql)
 
 # Sometimes 'spying' code is not acceptable even if it would be
 # disabled by default. That option allows to remove the feedback
 # daemon from the build completely.
 option(ENABLE_FEEDBACK_DAEMON "Feedback daemon which reports debug data to the Tarantool team" ON)
 
-add_subdirectory(sql)
-
 set(lua_sources)
 lua_source(lua_sources lua/load_cfg.lua)
 lua_source(lua_sources lua/schema.lua)
@@ -25,6 +24,52 @@ lua_source(lua_sources lua/merger.lua)
 set(bin_sources)
 bin_source(bin_sources bootstrap.snap bootstrap.h)
 
+set(sql_sources
+    sql/opcodes.c
+    sql/parse.c
+    sql/alter.c
+    sql/analyze.c
+    sql/cursor.c
+    sql/build.c
+    sql/callback.c
+    sql/date.c
+    sql/delete.c
+    sql/expr.c
+    sql/fk_constraint.c
+    sql/func.c
+    sql/global.c
+    sql/hash.c
+    sql/insert.c
+    sql/legacy.c
+    sql/main.c
+    sql/malloc.c
+    sql/os.c
+    sql/os_unix.c
+    sql/parse_def.c
+    sql/pragma.c
+    sql/prepare.c
+    sql/printf.c
+    sql/random.c
+    sql/resolve.c
+    sql/select.c
+    sql/tokenize.c
+    sql/treeview.c
+    sql/trigger.c
+    sql/utf.c
+    sql/update.c
+    sql/util.c
+    sql/vdbe.c
+    sql/vdbeapi.c
+    sql/vdbeaux.c
+    sql/vdbemem.c
+    sql/vdbesort.c
+    sql/vdbetrace.c
+    sql/walker.c
+    sql/where.c
+    sql/wherecode.c
+    sql/whereexpr.c
+)
+
 add_custom_target(box_generate_lua_sources
     WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/src/box
     DEPENDS ${lua_sources})
@@ -143,6 +188,7 @@ add_library(box STATIC
     wal.c
     call.c
     merger.c
+    ${sql_sources}
     ${lua_sources}
     lua/init.c
     lua/call.c
@@ -167,6 +213,53 @@ add_library(box STATIC
     lua/merger.c
     ${bin_sources})
 
+if(CMAKE_BUILD_TYPE STREQUAL "Debug")
+  add_definitions(-DSQL_DEBUG=1)
+endif()
+add_definitions(-DSQL_OMIT_AUTOMATIC_INDEX=1 -DSQL_TEST=1)
+
+set(EXT_SRC_DIR ${CMAKE_SOURCE_DIR}/extra)
+set(EXT_BIN_DIR ${CMAKE_BINARY_DIR}/extra)
+set(SQL_SRC_DIR ${CMAKE_SOURCE_DIR}/src/box/sql)
+set(SQL_BIN_DIR ${CMAKE_BINARY_DIR}/src/box/sql)
+
+include_directories(${SQL_SRC_DIR})
+include_directories(${SQL_BIN_DIR})
+
+add_custom_target(generate_sql_files DEPENDS
+    sql/parse.h
+    sql/keywordhash.h
+    sql/parse.y
+    sql/parse.c
+    sql/opcodes.c)
+
+add_custom_command(OUTPUT ${SQL_BIN_DIR}/keywordhash.h
+    COMMAND ${EXT_BIN_DIR}/mkkeywordhash > keywordhash.h.tmp
+    COMMAND ${CMAKE_COMMAND} -E copy_if_different keywordhash.h.tmp keywordhash.h
+    COMMAND ${CMAKE_COMMAND} -E remove keywordhash.h.tmp
+    WORKING_DIRECTORY "${SQL_BIN_DIR}"
+    DEPENDS mkkeywordhash)
+
+add_custom_command(OUTPUT ${SQL_BIN_DIR}/parse.h ${SQL_BIN_DIR}/parse.c
+    COMMAND ${EXT_BIN_DIR}/lemon -T${EXT_SRC_DIR}/lempar.c ${SQL_SRC_DIR}/parse.y
+    COMMAND ${CMAKE_COMMAND} -E copy parse.h parse.h.tmp
+    COMMAND ${EXT_SRC_DIR}/addopcodes.sh parse.h.tmp > parse.h
+    COMMAND ${CMAKE_COMMAND} -E remove parse.h.tmp parse.out
+    WORKING_DIRECTORY "${SQL_BIN_DIR}"
+    DEPENDS lemon ${SQL_SRC_DIR}/parse.y)
+
+add_custom_command(OUTPUT ${SQL_BIN_DIR}/opcodes.h
+    COMMAND cat parse.h ${SQL_SRC_DIR}/vdbe.c | ${EXT_SRC_DIR}/mkopcodeh.sh > opcodes.h
+    WORKING_DIRECTORY "${SQL_BIN_DIR}"
+    DEPENDS ${SQL_SRC_DIR}/vdbe.c ${EXT_SRC_DIR}/mkopcodeh.sh ${SQL_BIN_DIR}/parse.h)
+
+add_custom_command(OUTPUT ${SQL_BIN_DIR}/opcodes.c
+    COMMAND ${EXT_SRC_DIR}/mkopcodec.sh opcodes.h > opcodes.c
+    WORKING_DIRECTORY "${SQL_BIN_DIR}"
+    DEPENDS ${SQL_SRC_DIR}/vdbe.c ${EXT_SRC_DIR}/mkopcodec.sh ${SQL_BIN_DIR}/parse.h
+        ${SQL_BIN_DIR}/opcodes.h)
+
 target_link_libraries(box box_error tuple stat xrow xlog vclock crc32 scramble
-                      sql ${common_libraries})
-add_dependencies(box build_bundled_libs)
+                      ${common_libraries})
+
+add_dependencies(box build_bundled_libs generate_sql_files)
diff --git a/src/box/sql/CMakeLists.txt b/src/box/sql/CMakeLists.txt
deleted file mode 100644
index 1f2a6640f..000000000
--- a/src/box/sql/CMakeLists.txt
+++ /dev/null
@@ -1,109 +0,0 @@
-if(CMAKE_BUILD_TYPE STREQUAL "Debug")
-  add_definitions(-DSQL_DEBUG=1)
-endif()
-
-set(EXT_SRC_DIR ${CMAKE_SOURCE_DIR}/extra)
-set(EXT_BIN_DIR ${CMAKE_BINARY_DIR}/extra)
-set(SQL_SRC_DIR ${CMAKE_SOURCE_DIR}/src/box/sql)
-set(SQL_BIN_DIR ${CMAKE_BINARY_DIR}/src/box/sql)
-
-include_directories(${SQL_SRC_DIR})
-include_directories(${SQL_BIN_DIR})
-
-add_definitions(-DSQL_OMIT_AUTOMATIC_INDEX)
-
-set(TEST_DEFINITIONS
-    SQL_NO_SYNC=1
-    SQL_TEST=1
-    SQL_PRIVATE=
-    SQL_CORE=1
-)
-
-add_library(sql STATIC
-    # Generated files
-    opcodes.c
-    parse.c
-    alter.c
-    analyze.c
-    cursor.c
-    build.c
-    callback.c
-    date.c
-    delete.c
-    expr.c
-    fk_constraint.c
-    func.c
-    global.c
-    hash.c
-    insert.c
-    legacy.c
-    main.c
-    malloc.c
-    os.c
-    os_unix.c
-    parse_def.c
-    pragma.c
-    prepare.c
-    printf.c
-    random.c
-    resolve.c
-    select.c
-    tokenize.c
-    treeview.c
-    trigger.c
-    utf.c
-    update.c
-    util.c
-    vdbe.c
-    vdbeapi.c
-    vdbeaux.c
-    vdbemem.c
-    vdbesort.c
-    vdbetrace.c
-    walker.c
-    where.c
-    wherecode.c
-    whereexpr.c
-)
-set_target_properties(sql PROPERTIES COMPILE_DEFINITIONS
-    "${TEST_DEFINITIONS}")
-target_link_libraries(sql ${ICU_LIBRARIES})
-
-add_custom_target(generate_sql_files DEPENDS
-    parse.h
-    keywordhash.h
-    parse.y
-    parse.c
-    opcodes.c)
-
-add_custom_command(OUTPUT ${SQL_BIN_DIR}/keywordhash.h
-    COMMAND ${EXT_BIN_DIR}/mkkeywordhash > keywordhash.h.tmp
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different keywordhash.h.tmp keywordhash.h
-    COMMAND ${CMAKE_COMMAND} -E remove keywordhash.h.tmp
-    WORKING_DIRECTORY "${SQL_BIN_DIR}"
-    DEPENDS mkkeywordhash)
-
-add_custom_command(OUTPUT ${SQL_BIN_DIR}/parse.h ${SQL_BIN_DIR}/parse.c
-    COMMAND ${EXT_BIN_DIR}/lemon -T${EXT_SRC_DIR}/lempar.c ${SQL_SRC_DIR}/parse.y
-    COMMAND ${CMAKE_COMMAND} -E copy parse.h parse.h.tmp
-    COMMAND ${EXT_SRC_DIR}/addopcodes.sh parse.h.tmp > parse.h
-    COMMAND ${CMAKE_COMMAND} -E remove parse.h.tmp parse.out
-    WORKING_DIRECTORY "${SQL_BIN_DIR}"
-    DEPENDS lemon ${SQL_SRC_DIR}/parse.y)
-
-add_custom_command(OUTPUT ${SQL_BIN_DIR}/opcodes.h
-    COMMAND cat parse.h ${SQL_SRC_DIR}/vdbe.c | ${EXT_SRC_DIR}/mkopcodeh.sh > opcodes.h
-    WORKING_DIRECTORY "${SQL_BIN_DIR}"
-    DEPENDS ${SQL_SRC_DIR}/vdbe.c ${EXT_SRC_DIR}/mkopcodeh.sh ${SQL_BIN_DIR}/parse.h)
-
-add_custom_command(OUTPUT ${SQL_BIN_DIR}/opcodes.c
-    COMMAND ${EXT_SRC_DIR}/mkopcodec.sh opcodes.h > opcodes.c
-    WORKING_DIRECTORY "${SQL_BIN_DIR}"
-    DEPENDS ${SQL_SRC_DIR}/vdbe.c ${EXT_SRC_DIR}/mkopcodec.sh ${SQL_BIN_DIR}/parse.h
-        ${SQL_BIN_DIR}/opcodes.h)
-
-add_dependencies(sql generate_sql_files)
-
-if (APPLE)
-    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -undefined suppress -flat_namespace")
-endif(APPLE)
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH 2/2] cmake: split UB sanitations into separate flags.

[Vladislav Shpilevoy]

Clang undefined behaviour sanitizer was turned on using
-fsanitize=undefined flag, which is supposed to turn on all the
sanitizations, except a few ones. Not needed sanitations were
turned off explicitly, using -fno-sanitize=<type> flags. However
appeared it does not work with some flags. For example,
nullability sanitations can't be turned off when
-fsanitize=undefined is used.

Nullability sanitations lead to lots of false-positive fails
such as typeof(*obj) where obj is NULL, or memcpy() with NULL
destination but 0 size.

The patch splits -fsanitize=undefined into separate flags and
never turns on nullability checks.

Part of #4609
---
 cmake/compiler.cmake | 44 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
index 6c0fa635c..6de8219a0 100644
--- a/cmake/compiler.cmake
+++ b/cmake/compiler.cmake
@@ -269,19 +269,55 @@ macro(enable_tnt_compile_flags)
         if (NOT CMAKE_COMPILER_IS_CLANG)
             message(FATAL_ERROR "Undefined behaviour sanitizer only available for clang")
         else()
-            set(SANITIZE_FLAGS "-fsanitize=undefined -fno-sanitize-recover=undefined")
+            string(JOIN "," SANITIZE_FLAGS
+                "alignment"
+                "bool"
+                "bounds"
+                "builtin"
+                "enum"
+                "float-cast-overflow"
+                "float-divide-by-zero"
+                "function"
+                "integer-divide-by-zero"
+                "return"
+                "shift"
+                "unreachable"
+                "vla-bound"
+            )
+
+            # Exclude "object-size".
+            # Gives compilation warnings when -O0 is used, which is always,
+            # because some tests build with -O0.
+
+            # Exclude "pointer-overflow".
             # Stailq data structure subtracts a positive value from NULL.
-            set(SANITIZE_FLAGS ${SANITIZE_FLAGS} -fno-sanitize=pointer-overflow)
+
+            # Exclude "vptr".
             # Intrusive data structures may abuse '&obj->member' on pointer
             # 'obj' which is not really a pointer at an object of its type.
             # For example, rlist uses '&item->member' expression in macro cycles
             # to check end of cycle, but on the last iteration 'item' points at
             # the list metadata head, not at an object of type stored in this
             # list.
-            set(SANITIZE_FLAGS ${SANITIZE_FLAGS} -fno-sanitize=vptr)
+
+            # Exclude "implicit-signed-integer-truncation",
+            # "implicit-integer-sign-change", "signed-integer-overflow".
             # Integer overflow and truncation are disabled due to extensive
             # usage of this UB in SQL code to 'implement' some kind of int65_t.
-            set(SANITIZE_FLAGS ${SANITIZE_FLAGS} -fno-sanitize=implicit-signed-integer-truncation -fno-sanitize=implicit-integer-sign-change -fno-sanitize=signed-integer-overflow)
+
+            # Exclude "null", "nonnull-attribute", "nullability-arg",
+            # "returns-nonnull-attribute", "nullability-assign",
+            # "nullability-return".
+            # NULL checking is disabled, because this is not a UB and raises
+            # lots of false-positive fails such as typeof(*obj) with
+            # obj == NULL, or memcpy() with NULL argument and 0 size. All
+            # nullability sanitations are disabled, because from the tests it
+            # seems they implicitly turn each other on, when one is used. For
+            # example, having "returns-nonnull-attribute" may lead to fail in
+            # the typeof(*obj) when obj is NULL, even though there is nothing
+            # related to return.
+
+            set(SANITIZE_FLAGS "-fsanitize=${SANITIZE_FLAGS} -fno-sanitize-recover=${SANITIZE_FLAGS}")
 
             add_compile_flags("C;CXX" "${SANITIZE_FLAGS}")
         endif()
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 0/2] ASAN build

[Alexander Turenko]

LGTM except broken coverage reporting:

https://travis-ci.org/github/tarantool/tarantool/jobs/698246927

It looks quite similar to
https://github.com/tarantool/tarantool/commit/415c05868f95b45a56904d5aa581358aced05efe

Maybe we should add '-o ${CMAKE_BINARY_DIR}/src/box/sql/parse.c' to
lemon call to write absolute path in #line directives, which may help to
avoid ambiguity.

WBR, Alexander Turenko.

On Sun, Jun 14, 2020 at 06:24:29PM +0200, Vladislav Shpilevoy wrote:
> The build was broken because sql static library couldn't find some
> box symbols, when UB sanitizer was enabled.
> 
> Appeared, that the whole sql library was broken, because there was
> a cyclic dependency between box and sql libraries.
> 
> The patch merged sql and box libraries.
> 
> After their merge appeared that some UB sanitations 'woken up' and
> started failing, about nullability attributes and usage of NULL
> pointers. All of them were false-positive so the second patch
> disabled them.
> 
> After this patchset the build works, but the tests don't pass
> because of this:
> https://github.com/tarantool/tarantool/issues/5078.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5067-asan-build
> Issue: https://github.com/tarantool/tarantool/issues/5067
> Issue: https://github.com/tarantool/tarantool/issues/4609
> 
> Vladislav Shpilevoy (2):
>   sql: don't build sql as a separate library
>   cmake: split UB sanitations into separate flags.
> 
>  cmake/compiler.cmake       |  44 +++++++++++++--
>  src/CMakeLists.txt         |   2 +-
>  src/box/CMakeLists.txt     | 101 ++++++++++++++++++++++++++++++++--
>  src/box/sql/CMakeLists.txt | 109 -------------------------------------
>  4 files changed, 138 insertions(+), 118 deletions(-)
>  delete mode 100644 src/box/sql/CMakeLists.txt
> 
> -- 
> 2.21.1 (Apple Git-122.3)
> 

Re: [Tarantool-patches] [PATCH 2/2] cmake: split UB sanitations into separate flags.

[Timur Safin]

: Sent: Sunday, June 14, 2020 7:25 PM
: Subject: [PATCH 2/2] cmake: split UB sanitations into separate flags.
: 


: diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
: index 6c0fa635c..6de8219a0 100644
: --- a/cmake/compiler.cmake
: +++ b/cmake/compiler.cmake
: @@ -269,19 +269,55 @@ macro(enable_tnt_compile_flags)
:          if (NOT CMAKE_COMPILER_IS_CLANG)
:              message(FATAL_ERROR "Undefined behaviour sanitizer only
: available for clang")
:          else()
: -            set(SANITIZE_FLAGS "-fsanitize=undefined -fno-sanitize-
: recover=undefined")
: +            string(JOIN "," SANITIZE_FLAGS
: +                "alignment"
: +                "bool"
: +                "bounds"
: +                "builtin"
: +                "enum"
: +                "float-cast-overflow"
: +                "float-divide-by-zero"
: +                "function"
: +                "integer-divide-by-zero"
: +                "return"
: +                "shift"
: +                "unreachable"
: +                "vla-bound"
: +            )
: +

You know, every time I see (unnecessary) quoted strings inside of cmake 
lists I want to run and fix it immediately. Because there is no actual
need to quote them - in cmake almost everything is string literal at the end

Thus I tried to make this construction compacter and less verbose, e.g.

diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake                                              
index 6de8219a0..5a1141ebd 100644                                                                     
--- a/cmake/compiler.cmake                                                                            
+++ b/cmake/compiler.cmake                                                                            
@@ -270,19 +270,9 @@ macro(enable_tnt_compile_flags)                                                  
             message(FATAL_ERROR "Undefined behaviour sanitizer only available for clang")            
         else()                                                                                       
             string(JOIN "," SANITIZE_FLAGS                                                           
-                "alignment"                                                                          
-                "bool"                                                                               
-                "bounds"                                                                             
-                "builtin"                                                                            
-                "enum"                                                                               
-                "float-cast-overflow"                                                                
-                "float-divide-by-zero"                                                               
-                "function"                                                                           
-                "integer-divide-by-zero"                                                             
-                "return"                                                                             
-                "shift"                                                                              
-                "unreachable"                                                                        
-                "vla-bound"                                                                          
+                alignment bool bounds builtin enum float-cast-overflow                               
+                float-divide-by-zero function integer-divide-by-zero return                          
+                shift unreachable vla-bound                                                          
             )                                                                                        
                                                                                                      
             # Exclude "object-size".                                                                 
                                                                                                      

---
But it did not add any readability penny, so I'm not insisting on using it.


Hence, LGTM 

Re: [Tarantool-patches] [PATCH 1/2] sql: don't build sql as a separate library

[Timur Safin]

Eventually we need to migrate to glob usage (despite to being not 
recommended by cmake authors, it's the only way to maintain 
readable cmakelists for the decently large projects). One day...

LGTM, in any case

:  bin_source(bin_sources bootstrap.snap bootstrap.h)
: 
: +set(sql_sources
: +    sql/opcodes.c
: +    sql/parse.c
: +    sql/alter.c
: +    sql/analyze.c
: +    sql/cursor.c
: +    sql/build.c
: +    sql/callback.c
: +    sql/date.c
: +    sql/delete.c
: +    sql/expr.c
: +    sql/fk_constraint.c
: +    sql/func.c
: +    sql/global.c
: +    sql/hash.c
: +    sql/insert.c
: +    sql/legacy.c
: +    sql/main.c
: +    sql/malloc.c
: +    sql/os.c
: +    sql/os_unix.c
: +    sql/parse_def.c
: +    sql/pragma.c
: +    sql/prepare.c
: +    sql/printf.c
: +    sql/random.c
: +    sql/resolve.c
: +    sql/select.c
: +    sql/tokenize.c
: +    sql/treeview.c
: +    sql/trigger.c
: +    sql/utf.c
: +    sql/update.c
: +    sql/util.c
: +    sql/vdbe.c
: +    sql/vdbeapi.c
: +    sql/vdbeaux.c
: +    sql/vdbemem.c
: +    sql/vdbesort.c
: +    sql/vdbetrace.c
: +    sql/walker.c
: +    sql/where.c
: +    sql/wherecode.c
: +    sql/whereexpr.c
: +)
: +
:  add_custom_target(box_generate_lua_sources
:      WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/src/box
:      DEPENDS ${lua_sources})

Re: [Tarantool-patches] [PATCH 0/2] ASAN build

[Timur Safin]

LGTM

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Sunday, June 14, 2020 7:24 PM
: To: tarantool-patches@dev.tarantool.org; tsafin@tarantool.org
: Subject: [PATCH 0/2] ASAN build
: 
: The build was broken because sql static library couldn't find some
: box symbols, when UB sanitizer was enabled.
: 
: Appeared, that the whole sql library was broken, because there was
: a cyclic dependency between box and sql libraries.
: 
: The patch merged sql and box libraries.
: 
: After their merge appeared that some UB sanitations 'woken up' and
: started failing, about nullability attributes and usage of NULL
: pointers. All of them were false-positive so the second patch
: disabled them.
: 
: After this patchset the build works, but the tests don't pass
: because of this:
: https://github.com/tarantool/tarantool/issues/5078.
: 
: Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5067-asan-
: build
: Issue: https://github.com/tarantool/tarantool/issues/5067
: Issue: https://github.com/tarantool/tarantool/issues/4609
: 
: Vladislav Shpilevoy (2):
:   sql: don't build sql as a separate library
:   cmake: split UB sanitations into separate flags.
: 
:  cmake/compiler.cmake       |  44 +++++++++++++--
:  src/CMakeLists.txt         |   2 +-
:  src/box/CMakeLists.txt     | 101 ++++++++++++++++++++++++++++++++--
:  src/box/sql/CMakeLists.txt | 109 -------------------------------------
:  4 files changed, 138 insertions(+), 118 deletions(-)
:  delete mode 100644 src/box/sql/CMakeLists.txt
: 
: --
: 2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 2/2] cmake: split UB sanitations into separate flags.

[Vladislav Shpilevoy]

Hi! Thanks for the review!

> : diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake
> : index 6c0fa635c..6de8219a0 100644
> : --- a/cmake/compiler.cmake
> : +++ b/cmake/compiler.cmake
> : @@ -269,19 +269,55 @@ macro(enable_tnt_compile_flags)
> :          if (NOT CMAKE_COMPILER_IS_CLANG)
> :              message(FATAL_ERROR "Undefined behaviour sanitizer only
> : available for clang")
> :          else()
> : -            set(SANITIZE_FLAGS "-fsanitize=undefined -fno-sanitize-
> : recover=undefined")
> : +            string(JOIN "," SANITIZE_FLAGS
> : +                "alignment"
> : +                "bool"
> : +                "bounds"
> : +                "builtin"
> : +                "enum"
> : +                "float-cast-overflow"
> : +                "float-divide-by-zero"
> : +                "function"
> : +                "integer-divide-by-zero"
> : +                "return"
> : +                "shift"
> : +                "unreachable"
> : +                "vla-bound"
> : +            )
> : +
> 
> You know, every time I see (unnecessary) quoted strings inside of cmake 
> lists I want to run and fix it immediately. Because there is no actual
> need to quote them - in cmake almost everything is string literal at the end
> 
> Thus I tried to make this construction compacter and less verbose, e.g.

I didn't know the quotes are not needed. Nice! Applied your diff.

Re: [Tarantool-patches] [PATCH 0/2] ASAN build

[Vladislav Shpilevoy]

Hi!

On 15/06/2020 16:01, Alexander Turenko wrote:
> LGTM except broken coverage reporting:
> 
> https://travis-ci.org/github/tarantool/tarantool/jobs/698246927
> 
> It looks quite similar to
> https://github.com/tarantool/tarantool/commit/415c05868f95b45a56904d5aa581358aced05efe
> 
> Maybe we should add '-o ${CMAKE_BINARY_DIR}/src/box/sql/parse.c' to
> lemon call to write absolute path in #line directives, which may help to
> avoid ambiguity.

Thanks for noticing and for the proposed fix. I did this:

-    COMMAND ${EXT_BIN_DIR}/lemon -T${EXT_SRC_DIR}/lempar.c ${SQL_SRC_DIR}/parse.y
+    COMMAND ${EXT_BIN_DIR}/lemon -T${EXT_SRC_DIR}/lempar.c -o${SQL_BIN_DIR}/parse ${SQL_SRC_DIR}/parse.y

Now parse.c contains absolute paths in #line directives. But I couldn't
wait if it helped - travis is awfully slow. Lets see if it helped tomorrow,
when the job will be finally done.

Re: [Tarantool-patches] [PATCH 0/2] ASAN build

[Alexander Turenko]

On Tue, Jun 16, 2020 at 12:21:13AM +0200, Vladislav Shpilevoy wrote:
> Hi!
> 
> On 15/06/2020 16:01, Alexander Turenko wrote:
> > LGTM except broken coverage reporting:
> > 
> > https://travis-ci.org/github/tarantool/tarantool/jobs/698246927
> > 
> > It looks quite similar to
> > https://github.com/tarantool/tarantool/commit/415c05868f95b45a56904d5aa581358aced05efe
> > 
> > Maybe we should add '-o ${CMAKE_BINARY_DIR}/src/box/sql/parse.c' to
> > lemon call to write absolute path in #line directives, which may help to
> > avoid ambiguity.
> 
> Thanks for noticing and for the proposed fix. I did this:
> 
> -    COMMAND ${EXT_BIN_DIR}/lemon -T${EXT_SRC_DIR}/lempar.c ${SQL_SRC_DIR}/parse.y
> +    COMMAND ${EXT_BIN_DIR}/lemon -T${EXT_SRC_DIR}/lempar.c -o${SQL_BIN_DIR}/parse ${SQL_SRC_DIR}/parse.y

Nit: I would use `-o${SQL_BIN_DIR}/parse.c` with explicit `.c` suffix.
The effect is the same, but the command line looks more natural. Up to
you.

> 
> Now parse.c contains absolute paths in #line directives. But I couldn't
> wait if it helped - travis is awfully slow. Lets see if it helped tomorrow,
> when the job will be finally done.

You may verify it locally:

$ cmake .                        \
    -DCMAKE_BUILD_TYPE=Debug     \
    -DENABLE_BACKTRACE=ON        \
    -DENABLE_DIST=ON             \
    -DENABLE_BUNDLED_LIBCURL=OFF \
    -DENABLE_GCOV=ON
$ make -j
$ echo 'print(42)' | ./src/tarantool
$ lcov -c -d src -o /dev/null 2>&1 | grep WARNING

Note: It seems the output format may vary across lcov versions, I see no
WARNING mark on Travis-CI, just the following lines:

 | Processing box/CMakeFiles/box.dir/sql/parse.c.gcda
 | Cannot open source file parse.c

After your fix the warning disappears.

BTW, I see warnings re uri.rl / uri.c locally again:

 | geninfo: WARNING: could not open /home/alex/p/tarantool-meta/r/t-3/src/lib/uri/src/lib/uri/uri.rl
 | geninfo: WARNING: could not open /home/alex/p/tarantool-meta/r/t-3/src/lib/uri/src/lib/uri/uri.c
 | geninfo: WARNING: some exclusion markers may be ignored

But don't see them on Travis-CI. This is strange. But it doesn't look
related to your patchset: I see the same on master
(2.5.0-142-ged935572b).

Re: [Tarantool-patches] [PATCH 0/2] ASAN build

[Vladislav Shpilevoy]

On 16/06/2020 01:04, Alexander Turenko wrote:
> On Tue, Jun 16, 2020 at 12:21:13AM +0200, Vladislav Shpilevoy wrote:
>> Hi!
>>
>> On 15/06/2020 16:01, Alexander Turenko wrote:
>>> LGTM except broken coverage reporting:
>>>
>>> https://travis-ci.org/github/tarantool/tarantool/jobs/698246927
>>>
>>> It looks quite similar to
>>> https://github.com/tarantool/tarantool/commit/415c05868f95b45a56904d5aa581358aced05efe
>>>
>>> Maybe we should add '-o ${CMAKE_BINARY_DIR}/src/box/sql/parse.c' to
>>> lemon call to write absolute path in #line directives, which may help to
>>> avoid ambiguity.
>>
>> Thanks for noticing and for the proposed fix. I did this:
>>
>> -    COMMAND ${EXT_BIN_DIR}/lemon -T${EXT_SRC_DIR}/lempar.c ${SQL_SRC_DIR}/parse.y
>> +    COMMAND ${EXT_BIN_DIR}/lemon -T${EXT_SRC_DIR}/lempar.c -o${SQL_BIN_DIR}/parse ${SQL_SRC_DIR}/parse.y
> 
> Nit: I would use `-o${SQL_BIN_DIR}/parse.c` with explicit `.c` suffix.
> The effect is the same, but the command line looks more natural. Up to
> you.

Changed to parse.c.

Re: [Tarantool-patches] [PATCH 0/2] ASAN build

[Kirill Yukhin]

Hello,

On 14 июн 18:24, Vladislav Shpilevoy wrote:
> The build was broken because sql static library couldn't find some
> box symbols, when UB sanitizer was enabled.
> 
> Appeared, that the whole sql library was broken, because there was
> a cyclic dependency between box and sql libraries.
> 
> The patch merged sql and box libraries.
> 
> After their merge appeared that some UB sanitations 'woken up' and
> started failing, about nullability attributes and usage of NULL
> pointers. All of them were false-positive so the second patch
> disabled them.
> 
> After this patchset the build works, but the tests don't pass
> because of this:
> https://github.com/tarantool/tarantool/issues/5078.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5067-asan-build
> Issue: https://github.com/tarantool/tarantool/issues/5067
> Issue: https://github.com/tarantool/tarantool/issues/4609

I've checked your patch set into master.

--
Regards, Kirill Yukhin