* [Tarantool-patches] [PATCH 1/2] sql: don't build sql as a separate library
2020-06-14 16:24 [Tarantool-patches] [PATCH 0/2] ASAN build Vladislav Shpilevoy
@ 2020-06-14 16:24 ` Vladislav Shpilevoy
2020-06-15 15:42 ` Timur Safin
2020-06-14 16:24 ` [Tarantool-patches] [PATCH 2/2] cmake: split UB sanitations into separate flags Vladislav Shpilevoy
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-14 16:24 UTC (permalink / raw)
To: tarantool-patches, tsafin
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)
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Tarantool-patches] [PATCH 1/2] sql: don't build sql as a separate library
2020-06-14 16:24 ` [Tarantool-patches] [PATCH 1/2] sql: don't build sql as a separate library Vladislav Shpilevoy
@ 2020-06-15 15:42 ` Timur Safin
0 siblings, 0 replies; 12+ messages in thread
From: Timur Safin @ 2020-06-15 15:42 UTC (permalink / raw)
To: 'Vladislav Shpilevoy', tarantool-patches
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})
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Tarantool-patches] [PATCH 2/2] cmake: split UB sanitations into separate flags.
2020-06-14 16:24 [Tarantool-patches] [PATCH 0/2] ASAN build Vladislav Shpilevoy
2020-06-14 16:24 ` [Tarantool-patches] [PATCH 1/2] sql: don't build sql as a separate library Vladislav Shpilevoy
@ 2020-06-14 16:24 ` Vladislav Shpilevoy
2020-06-15 15:41 ` Timur Safin
2020-06-15 14:01 ` [Tarantool-patches] [PATCH 0/2] ASAN build Alexander Turenko
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-14 16:24 UTC (permalink / raw)
To: tarantool-patches, tsafin
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)
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Tarantool-patches] [PATCH 2/2] cmake: split UB sanitations into separate flags.
2020-06-14 16:24 ` [Tarantool-patches] [PATCH 2/2] cmake: split UB sanitations into separate flags Vladislav Shpilevoy
@ 2020-06-15 15:41 ` Timur Safin
2020-06-15 22:19 ` Vladislav Shpilevoy
0 siblings, 1 reply; 12+ messages in thread
From: Timur Safin @ 2020-06-15 15:41 UTC (permalink / raw)
To: 'Vladislav Shpilevoy', tarantool-patches
: 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Tarantool-patches] [PATCH 2/2] cmake: split UB sanitations into separate flags.
2020-06-15 15:41 ` Timur Safin
@ 2020-06-15 22:19 ` Vladislav Shpilevoy
0 siblings, 0 replies; 12+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-15 22:19 UTC (permalink / raw)
To: Timur Safin, tarantool-patches
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/2] ASAN build
2020-06-14 16:24 [Tarantool-patches] [PATCH 0/2] ASAN build Vladislav Shpilevoy
2020-06-14 16:24 ` [Tarantool-patches] [PATCH 1/2] sql: don't build sql as a separate library Vladislav Shpilevoy
2020-06-14 16:24 ` [Tarantool-patches] [PATCH 2/2] cmake: split UB sanitations into separate flags Vladislav Shpilevoy
@ 2020-06-15 14:01 ` Alexander Turenko
2020-06-15 22:21 ` Vladislav Shpilevoy
2020-06-15 15:43 ` Timur Safin
2020-06-16 8:56 ` Kirill Yukhin
4 siblings, 1 reply; 12+ messages in thread
From: Alexander Turenko @ 2020-06-15 14:01 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
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)
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Tarantool-patches] [PATCH 0/2] ASAN build
2020-06-15 14:01 ` [Tarantool-patches] [PATCH 0/2] ASAN build Alexander Turenko
@ 2020-06-15 22:21 ` Vladislav Shpilevoy
2020-06-15 23:04 ` Alexander Turenko
0 siblings, 1 reply; 12+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-15 22:21 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Tarantool-patches] [PATCH 0/2] ASAN build
2020-06-15 22:21 ` Vladislav Shpilevoy
@ 2020-06-15 23:04 ` Alexander Turenko
2020-06-15 23:15 ` Vladislav Shpilevoy
0 siblings, 1 reply; 12+ messages in thread
From: Alexander Turenko @ 2020-06-15 23:04 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
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).
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/2] ASAN build
2020-06-14 16:24 [Tarantool-patches] [PATCH 0/2] ASAN build Vladislav Shpilevoy
` (2 preceding siblings ...)
2020-06-15 14:01 ` [Tarantool-patches] [PATCH 0/2] ASAN build Alexander Turenko
@ 2020-06-15 15:43 ` Timur Safin
2020-06-16 8:56 ` Kirill Yukhin
4 siblings, 0 replies; 12+ messages in thread
From: Timur Safin @ 2020-06-15 15:43 UTC (permalink / raw)
To: 'Vladislav Shpilevoy', tarantool-patches
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)
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Tarantool-patches] [PATCH 0/2] ASAN build
2020-06-14 16:24 [Tarantool-patches] [PATCH 0/2] ASAN build Vladislav Shpilevoy
` (3 preceding siblings ...)
2020-06-15 15:43 ` Timur Safin
@ 2020-06-16 8:56 ` Kirill Yukhin
4 siblings, 0 replies; 12+ messages in thread
From: Kirill Yukhin @ 2020-06-16 8:56 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 12+ messages in thread