From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Alexander Turenko Subject: [PATCH v2] travis-ci: set right flags in release testing jobs Date: Mon, 29 Apr 2019 15:57:06 +0300 Message-Id: <5de0fe153c337e379087239bcce3b48e97b07f0f.1556542564.git.alexander.turenko@tarantool.org> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit To: Vladimir Davydov Cc: Alexander Turenko , tarantool-patches@freelists.org, Vladislav Shpilevoy List-ID: It is important to have testing jobs that build the project with both -Werror and -O2 to keep the code clean. -O2 is needed, because some compiler warnings are available only after extra analyzing passes that are disabled with lesser optimization levels. The first attempt to add -Werror for release testing jobs was made in da505ee72883195121b7006bbd52bc66e57321f3 ('Add -Werror for CI (1.10 part)'), but it mistakely doesn't enable -O2 for RelWithDebInfoWError build. It is possible to fix it in this way: | --- a/cmake/compiler.cmake | +++ b/cmake/compiler.cmake | @@ -113,10 +113,14 @@ set (CMAKE_C_FLAGS_DEBUG | "${CMAKE_C_FLAGS_DEBUG} ${CC_DEBUG_OPT} -O0") | set (CMAKE_C_FLAGS_RELWITHDEBINFO | "${CMAKE_C_FLAGS_RELWITHDEBINFO} ${CC_DEBUG_OPT} -O2") | +set (CMAKE_C_FLAGS_RELWITHDEBINFOWERROR | + "${CMAKE_C_FLAGS_RELWITHDEBINFOWERROR} ${CC_DEBUG_OPT} -O2") | set (CMAKE_CXX_FLAGS_DEBUG | "${CMAKE_CXX_FLAGS_DEBUG} ${CC_DEBUG_OPT} -O0") | set (CMAKE_CXX_FLAGS_RELWITHDEBINFO | "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} ${CC_DEBUG_OPT} -O2") | +set (CMAKE_CXX_FLAGS_RELWITHDEBINFOWERROR | + "${CMAKE_CXX_FLAGS_RELWITHDEBINFOWERROR} ${CC_DEBUG_OPT} -O2") | | unset(CC_DEBUG_OPT) However I think that a build type (and so `tarantool --version`) should not show whether -Werror was passed or not. So I have added ENABLE_WERROR CMake option for that. It can be set like so: | cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON Enabled the option in testing Travis-CI jobs with the RelWithDebInfo build type. Deploy jobs don't include it as before. Fixed all -Wmaybe-uninitialized and -Wunused-result warnings. A few notes about the fixes: * net.box does not validate received data in general, so I don't add a check for autoincrement IDs too. Set the ID to INT64_MIN, because this value is less probably will appear here in a normal case and so is the best one to signal a user that something probably going wrongly. * xrow_decode_*() functions could read uninitialized data from row->body[0].iov_base in xrow_on_decode_err() when printing a hex code for a row. It could be possible when the received msgpack was empty (row->bodycnt == 0), but there were expected keys (key_map != 0). * getcwd() is marked with __attribute__((__warn_unused_result__)) in glibc, but the buffer filled by this call is not used anywhere and so just removed. * Vinyl -Wmaybe-uninitialized warnings are false positive ones. Added comments and quotes into .travis.yml to ease reading. Removed "test" word from the CentOS 6 job name, because we don't run tests on this distro (disabled in the RPM spec). Fixes #4178. --- This patch targets all currently supported long-term branches. https://github.com/tarantool/tarantool/issues/4178 https://github.com/tarantool/tarantool/tree/Totktonada/gh-4178-set-right-flags-in-release-testing-jobs .travis.mk | 4 ++-- .travis.yml | 45 +++++++++++++++++++++++-------------------- cmake/compiler.cmake | 16 ++++++++++----- src/box/lua/net_box.c | 2 +- src/box/sql/func.c | 16 +++++++-------- src/box/vy_cache.c | 2 +- src/box/vy_mem.c | 2 +- src/box/xrow.c | 30 ++++++++++++++++++----------- src/lua/init.c | 2 -- 9 files changed, 67 insertions(+), 52 deletions(-) diff --git a/.travis.mk b/.travis.mk index 247bb2a2d..973c6a7c7 100644 --- a/.travis.mk +++ b/.travis.mk @@ -40,7 +40,7 @@ deps_ubuntu: lcov ruby test_ubuntu: deps_ubuntu - cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfoWError ${CMAKE_EXTRA_PARAMS} + cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS} make -j8 cd test && /usr/bin/python test-run.py -j 1 @@ -51,7 +51,7 @@ deps_osx: pip install -r test-run/requirements.txt test_osx: deps_osx - cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfoWError ${CMAKE_EXTRA_PARAMS} + cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS} # Increase the maximum number of open file descriptors on macOS sudo sysctl -w kern.maxfiles=20480 || : sudo sysctl -w kern.maxfilesperproc=20480 || : diff --git a/.travis.yml b/.travis.yml index e30540837..6e79cf2fe 100644 --- a/.travis.yml +++ b/.travis.yml @@ -19,69 +19,72 @@ git: jobs: include: - - name: RelWithDebInfoWError build + test (Linux, gcc) + # Testing targets (just run tests on Debian Stretch or OS X). + - name: "RelWithDebInfo build + test (Linux, gcc)" env: TARGET=test - - name: RelWithDebInfoWError build + test (Linux, clang) + - name: "RelWithDebInfo build + test (Linux, clang)" env: TARGET=test compiler: clang - - name: RelWithDebInfoWError build + test (OS X Mojave 10.14) + - name: "RelWithDebInfo build + test (OS X Mojave 10.14)" env: TARGET=test os: osx - - name: Debug build + test + coverage (Linux, gcc) + - name: "Debug build + test + coverage (Linux, gcc)" env: TARGET=coverage - - name: RelWithDebInfoWError build + test (OS X High Sierra 10.13) + - name: "RelWithDebInfo build + test (OS X High Sierra 10.13)" env: TARGET=test os: osx osx_image: xcode9.4 if: branch = "master" - - name: LTO build + test (Linux, gcc) + # Special targets (only LTO for now). + - name: "LTO build + test (Linux, gcc)" env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON if: branch = "master" - - name: LTO build + test (Linux, clang) + - name: "LTO build + test (Linux, clang)" env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON if: branch = "master" compiler: clang - - name: LTO build + test (OS X Mojave 10.14) + - name: "LTO build + test (OS X Mojave 10.14)" os: osx env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON if: branch = "master" - - name: Create and deploy tarball + # Deploy targets (they also catch distro-specific problems). + - name: "Create and deploy tarball" env: TARGET=source if: branch = "master" - - name: CentOS 6 build + test + deploy RPM + - name: "CentOS 6 build + deploy RPM" env: OS=el DIST=6 if: branch = "master" - - name: CentOS 7 build + test + deploy RPM + - name: "CentOS 7 build + test + deploy RPM" env: OS=el DIST=7 if: branch = "master" - - name: Fedora 28 build + test + deploy RPM + - name: "Fedora 28 build + test + deploy RPM" env: OS=fedora DIST=28 if: branch = "master" - - name: Fedora 29 build + test + deploy RPM + - name: "Fedora 29 build + test + deploy RPM" env: OS=fedora DIST=29 if: branch = "master" - - name: Ubuntu Trusty (14.04) build + deploy DEB + - name: "Ubuntu Trusty (14.04) build + deploy DEB" env: OS=ubuntu DIST=trusty if: branch = "master" - - name: Ubuntu Xenial (16.04) build + deploy DEB + - name: "Ubuntu Xenial (16.04) build + deploy DEB" env: OS=ubuntu DIST=xenial if: branch = "master" - - name: Ubuntu Bionic (18.04) build + deploy DEB + - name: "Ubuntu Bionic (18.04) build + deploy DEB" env: OS=ubuntu DIST=bionic if: branch = "master" - - name: Ubuntu Cosmic (18.10) build + deploy DEB + - name: "Ubuntu Cosmic (18.10) build + deploy DEB" env: OS=ubuntu DIST=cosmic if: branch = "master" - - name: Ubuntu Disco (19.04) build + deploy DEB + - name: "Ubuntu Disco (19.04) build + deploy DEB" env: OS=ubuntu DIST=disco if: branch = "master" - - name: Debian Jessie (8) build + deploy DEB + - name: "Debian Jessie (8) build + deploy DEB" env: OS=debian DIST=jessie if: branch = "master" - - name: Debian Stretch (9) build + deploy DEB + - name: "Debian Stretch (9) build + deploy DEB" env: OS=debian DIST=stretch if: branch = "master" - - name: Debian Buster (10) build + deploy DEB + - name: "Debian Buster (10) build + deploy DEB" env: OS=debian DIST=buster if: branch = "master" diff --git a/cmake/compiler.cmake b/cmake/compiler.cmake index 4062d13ec..887485c80 100644 --- a/cmake/compiler.cmake +++ b/cmake/compiler.cmake @@ -223,6 +223,8 @@ if (NOT CMAKE_BUILD_TYPE STREQUAL "Debug") add_definitions("-DNDEBUG" "-DNVALGRIND") endif() +option(ENABLE_WERROR "Make all compiler warnings into errors" OFF) + macro(enable_tnt_compile_flags) # Tarantool code is written in GNU C dialect. # Additionally, compile it with more strict flags than the rest @@ -286,11 +288,15 @@ macro(enable_tnt_compile_flags) add_definitions("-D__STDC_LIMIT_MACROS=1") add_definitions("-D__STDC_CONSTANT_MACROS=1") - # Only add -Werror if it's a Debug or RelWithDebInfoWError build, done by - # developers. Release builds should not cause extra trouble. - if (((${CMAKE_BUILD_TYPE} STREQUAL "Debug") OR - (${CMAKE_BUILD_TYPE} STREQUAL "RelWithDebInfoWError")) AND - HAVE_STD_C11 AND HAVE_STD_CXX11) + # Only add -Werror if it's a debug build, done by developers. + # Release builds should not cause extra trouble. + if ((${CMAKE_BUILD_TYPE} STREQUAL "Debug") + AND HAVE_STD_C11 AND HAVE_STD_CXX11) + add_compile_flags("C;CXX" "-Werror") + endif() + + # Add -Werror if it is requested explicitly. + if (ENABLE_WERROR) add_compile_flags("C;CXX" "-Werror") endif() endmacro(enable_tnt_compile_flags) diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c index 2dfc4fc9d..7484a86bf 100644 --- a/src/box/lua/net_box.c +++ b/src/box/lua/net_box.c @@ -694,7 +694,7 @@ netbox_decode_sql_info(struct lua_State *L, const char **data) assert(count > 0); lua_createtable(L, 0, count); for (uint32_t j = 0; j < count; ++j) { - int64_t id; + int64_t id = INT64_MIN; mp_read_int64(data, &id); luaL_pushint64(L, id); lua_rawseti(L, -2, j + 1); diff --git a/src/box/sql/func.c b/src/box/sql/func.c index f3790ba01..bb7405e68 100644 --- a/src/box/sql/func.c +++ b/src/box/sql/func.c @@ -758,15 +758,15 @@ sql_utf8_pattern_compare(const char *pattern, SQL_END_OF_STRING) { if (c == SQL_INVALID_UTF8_SYMBOL) return INVALID_PATTERN; - if (c != MATCH_ALL_WILDCARD && - c != MATCH_ONE_WILDCARD) + if (c == MATCH_ONE_WILDCARD) { + c2 = Utf8Read(string, string_end); + if (c2 == SQL_INVALID_UTF8_SYMBOL) + return NO_MATCH; + if (c2 == SQL_END_OF_STRING) + return NO_WILDCARD_MATCH; + } else if (c != MATCH_ALL_WILDCARD) { break; - if (c == MATCH_ONE_WILDCARD && - (c2 = Utf8Read(string, string_end)) == - SQL_END_OF_STRING) - return NO_WILDCARD_MATCH; - if (c2 == SQL_INVALID_UTF8_SYMBOL) - return NO_MATCH; + } } /* * "%" at the end of the pattern matches. diff --git a/src/box/vy_cache.c b/src/box/vy_cache.c index be11b986f..7007d0e35 100644 --- a/src/box/vy_cache.c +++ b/src/box/vy_cache.c @@ -661,7 +661,7 @@ vy_cache_iterator_seek(struct vy_cache_iterator *itr, struct vy_entry last) ITER_GT : ITER_LT; } - bool exact; + bool exact = false; if (!vy_stmt_is_empty_key(key.stmt)) { itr->curr_pos = iterator_type == ITER_EQ || iterator_type == ITER_GE || diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c index 9513d5b80..a4fae26e2 100644 --- a/src/box/vy_mem.c +++ b/src/box/vy_mem.c @@ -392,7 +392,7 @@ vy_mem_iterator_seek(struct vy_mem_iterator *itr, struct vy_entry last) ITER_GT : ITER_LT; } - bool exact; + bool exact = false; struct vy_mem_tree_key tree_key; tree_key.entry = key; /* (lsn == INT64_MAX - 1) means that lsn is ignored in comparison */ diff --git a/src/box/xrow.c b/src/box/xrow.c index aaed84e38..a3c9b3218 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -509,9 +509,12 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version, schema_version, sizeof(body) + msg_len); body.v_data_len = mp_bswap_u32(msg_len); - (void) write(fd, header, sizeof(header)); - (void) write(fd, &body, sizeof(body)); - (void) write(fd, e->errmsg, msg_len); + + ssize_t unused; + unused = write(fd, header, sizeof(header)); + unused = write(fd, &body, sizeof(body)); + unused = write(fd, e->errmsg, msg_len); + (void) unused; } int @@ -624,12 +627,15 @@ xrow_decode_dml(struct xrow_header *row, struct request *request, request->header = row; request->type = row->type; + const char *start = NULL; + const char *end = NULL; + if (row->bodycnt == 0) goto done; assert(row->bodycnt == 1); - const char *data = (const char *) row->body[0].iov_base; - const char *end = data + row->body[0].iov_len; + const char *data = start = (const char *) row->body[0].iov_base; + end = data + row->body[0].iov_len; assert((end - data) > 0); if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) { @@ -701,8 +707,8 @@ error: done: if (key_map) { enum iproto_key key = (enum iproto_key) bit_ctz_u64(key_map); - xrow_on_decode_err(row->body[0].iov_base, end, - ER_MISSING_REQUEST_FIELD, iproto_key_name(key)); + xrow_on_decode_err(start, end, ER_MISSING_REQUEST_FIELD, + iproto_key_name(key)); return -1; } return 0; @@ -1071,12 +1077,15 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot) ballot->is_ro = false; vclock_create(&ballot->vclock); + const char *start = NULL; + const char *end = NULL; + if (row->bodycnt == 0) goto err; assert(row->bodycnt == 1); - const char *data = (const char *) row->body[0].iov_base; - const char *end = data + row->body[0].iov_len; + const char *data = start = (const char *) row->body[0].iov_base; + end = data + row->body[0].iov_len; const char *tmp = data; if (mp_check(&tmp, end) != 0 || mp_typeof(*data) != MP_MAP) goto err; @@ -1124,8 +1133,7 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot) } return 0; err: - xrow_on_decode_err(row->body[0].iov_base, end, ER_INVALID_MSGPACK, - "packet body"); + xrow_on_decode_err(start, end, ER_INVALID_MSGPACK, "packet body"); return -1; } diff --git a/src/lua/init.c b/src/lua/init.c index d18c8af94..be164bdfb 100644 --- a/src/lua/init.c +++ b/src/lua/init.c @@ -318,8 +318,6 @@ static void tarantool_lua_setpaths(struct lua_State *L) { const char *home = getenv("HOME"); - char cwd[PATH_MAX] = {'\0'}; - getcwd(cwd, sizeof(cwd)); lua_getglobal(L, "package"); int top = lua_gettop(L); -- 2.21.0