Tarantool development patches archive
 help / color / mirror / Atom feed
* [tarantool-patches] [PATCH] travis-ci: set right flags in release testing jobs
@ 2019-04-26  1:29 Alexander Turenko
  2019-04-26 10:18 ` [tarantool-patches] " Vladislav Shpilevoy
  0 siblings, 1 reply; 10+ messages in thread
From: Alexander Turenko @ 2019-04-26  1:29 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: Alexander Turenko, tarantool-patches

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 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.
* xrow_decode_*() functions could read uninitialized data when printing
  a hex code for a row if the received msgpack is empty, but there are
  expected keys.

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.
---

I'm a bit tentative about xrow.c changes. They are correct in the sense
that the code will work, but maybe there is a better way to eliminate
the warnings.

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 |  4 ++--
 src/box/sql/func.c    | 13 +++++++------
 src/box/vy_cache.c    |  2 +-
 src/box/vy_mem.c      |  2 +-
 src/box/xrow.c        | 33 ++++++++++++++++++++-----------
 src/lua/init.c        |  4 +++-
 9 files changed, 73 insertions(+), 50 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..14177aaab 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -689,12 +689,12 @@ netbox_decode_sql_info(struct lua_State *L, const char **data)
 	if (map_size == 2) {
 		key = mp_decode_uint(data);
 		assert(key == SQL_INFO_AUTOINCREMENT_IDS);
-		(void)key;
+		(void) key;
 		uint64_t count = mp_decode_array(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..f96709a75 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -761,12 +761,13 @@ sql_utf8_pattern_compare(const char *pattern,
 				if (c != MATCH_ALL_WILDCARD &&
 				    c != MATCH_ONE_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;
+				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;
+				}
 			}
 			/*
 			 * "%" 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..ccdd9def5 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -509,9 +509,14 @@ 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);
+
+	int rc1, rc2, rc3;
+	rc1 = write(fd, header, sizeof(header));
+	rc2 = write(fd, &body, sizeof(body));
+	rc3 = write(fd, e->errmsg, msg_len);
+	(void) rc1;
+	(void) rc2;
+	(void) rc3;
 }
 
 int
@@ -624,12 +629,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 +709,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 +1079,16 @@ 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 +1136,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..f66a3eb46 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -319,7 +319,9 @@ tarantool_lua_setpaths(struct lua_State *L)
 {
 	const char *home = getenv("HOME");
 	char cwd[PATH_MAX] = {'\0'};
-	getcwd(cwd, sizeof(cwd));
+	char *rc = getcwd(cwd, sizeof(cwd));
+	if (rc == NULL)
+		panic("can't get cwd; errno: %d", errno);
 	lua_getglobal(L, "package");
 	int top = lua_gettop(L);
 
-- 
2.21.0

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-29 17:09 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-26  1:29 [tarantool-patches] [PATCH] travis-ci: set right flags in release testing jobs Alexander Turenko
2019-04-26 10:18 ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-28  5:40   ` Alexander Turenko
2019-04-28  8:54     ` Vladislav Shpilevoy
2019-04-28  9:11       ` Vladislav Shpilevoy
2019-04-28 21:38         ` Alexander Turenko
2019-04-28 21:35       ` Alexander Turenko
2019-04-29 10:07         ` Vladislav Shpilevoy
2019-04-29 12:57           ` [PATCH v2] " Alexander Turenko
2019-04-29 17:09             ` Vladimir Davydov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox