[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Alexander Turenko <alexander.turenko@tarantool.org>]

Hi!

Thanks for your time. Answered inline.

Pasted diff from my previous commit at end of the email.

WBR, Alexander Turenko.

On Fri, Apr 26, 2019 at 01:18:28PM +0300, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> Firstly, after your patch I can't build Tarantool with gcc-7
> on Mac with this new -DENABLE_WERROR=ON option. I get these
> errors:
> 
> error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
> errorScanning dependencies of target rope_stress.test
> error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
> error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
> error: unknown warning option '-Wno-format-truncation' [-Werror,-Wunknown-warning-option]
> error: unknown warning option '-Wno-cast-function-type'; did you mean '-Wno-bad-function-cast'? [-Werror,-Wunknown-warning-option]
> 
> After that I found that master branch also can't be built, but
> for another reason:
> https://github.com/tarantool/tarantool/issues/4184
> 
> I think, that your commit should not exacerbate it, at least.

GCC ignores unknown -W... options to allow to enable or disable warnings
that is introduced in newer GCC versions unconditionally. However when
some error occurs GCC reports about unknown -W... options.

This is just how GCC operates. You can safely ignore this output: it
will not appear when a build is successful.

> 
> See 9 comments below, and my fixes on the branch in a separate commit.
> 
> > 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;
> 
> 1. Unnecessary diff. Please, drop.

I don't think so. It is okay to fix a code block that you touch in a
commit to fix it according to a code style.

This would be applicable if I would change some unrelated block of code:
here I'm agree with you.

> 
> >  		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;
> 
> 2. Is there any concrete reason why the value is so
> non-trivial? Why not just simple zero?

We don't verify a data received from a network in net.box for
correctness. AFAIU from your words it was the design decision. So I
consider this broken by design, but try to at least set a value that
less probably appear in a normal output to signal a user that something
going wrong (when we get something other then int64 here).

Of course, we should give an error in the case, but I left this case
unverified to keep things consistent.

> 
> >  			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) {
> 
> 3. Now you check for 'c == MATCH_ONE_WILDCARD' two times: here and
> two lines above. Please, consider my fixes:
> 
> =====================================================================
>  			       SQL_END_OF_STRING) {
>  				if (c == SQL_INVALID_UTF8_SYMBOL)
>  					return INVALID_PATTERN;
> -				if (c != MATCH_ALL_WILDCARD &&
> -				    c != MATCH_ONE_WILDCARD)
> -					break;
>  				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;
>  				}
>  			}
>  			/*
> =====================================================================

Thanks. Applied.

> 
> > +					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;
> 
> 4. Sorry, can't review Vinyl part, because I do not understand this code.
> But anyway your solution is better than an uninitialized variable. By the way,
> can it be the problem Kirill investigates right now, when some tuples are
> not returned from 'select'?

Unlikely. This warning is false positive here and the patch does not
really change any behaviour around vinyl.

> 
> >  	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;
> 
> 5. Firstly, you could use one 'rc' for all of them. It would be shorter.
> Secondly, why not just remove these (void) casts? We ignore returned value
> in many places and it is ok, if it isn't really important.
> 
> In this concrete place an error has already happened, and it makes
> no sense to create one another - this is why the values are ignored.
> 
> =====================================================================
>  	body.v_data_len = mp_bswap_u32(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;
> +	write(fd, header, sizeof(header));
> +	write(fd, &body, sizeof(body));
> +	write(fd, e->errmsg, msg_len);
> =====================================================================

The write() glibc syscall wrapper is marked with
__attribute__((__warn_unused_result__)), so we cannot ignore this result
when -Werror is set. Maybe it is not so for your libc implementation /
version: in this case you'll not get this warning.

Re using one rc: I had the thought that clang tracks whether a result is
used or discarded with a next assingment, so I wrote the code in that
way. But I don't observe such behaviour with clang-6, 7, 8 and 3.9. It
seems I did remember it wrongly and a result is considered as 'used'
when we assign it; a cast to void is just to eliminate the 'variable set
but not used' warning.

GCC complains about (void) write(...);, so we need to assign a result
into a variable: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425

Rewritten as follows:

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 +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));
> 
> 6. Why did you add 'start' variable? This code was fully okay, as I
> understand. Compiler could not raise an error on 'row->body[0].iov_base'
> usage here. Please, drop this hunk. A patch should not contain unrelated
> changes making review harder and git history dirtier.

The compiler warning is not false positive here: the code was not okay.
I'll cite the commit message:

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

Please, suggest a change for wording if the current one is not clear.

My change causes `for (const char *cur = start; cur < end;) { ... }` in
dump_row_hex() to walk around the loop body and so prevents reading from
`start` in the loop (NULL < NULL is false).

> 
> >  		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;
> > +
> 
> 7. Excessive empty line. Please, drop.

Don't know which empty line you considered as excessive. If you can link
a guide clause about that or describe a formal rule you want to make
mandatory, then put it here. In the latter case it would be good to
strengthen it with examples from our code. (However I propose to don't
spend time on that: it is counter-productive.)

BTW, I looked at the code again and removed an empty line after
assert(row->bodycnt == 1); in xrow_decode_dml() to make corresponding
lines look as in xrow_decode_ballot().

> 
> >  	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");
> 
> 8. The same as 6.

Answered above.

> 
> >  	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);
> 
> 9. 'cwd' variable is not used at all and can be removed. It was
> introduced here 9e7c421758af156d19a66cebf713e24b42d8654b but
> was forgotten to remove sometime.
> 
> =====================================================================
>  	const char *home = getenv("HOME");
> -	char cwd[PATH_MAX] = {'\0'};
> -	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);
> =====================================================================

Nice catch, thanks. Removed.

> 
> >  	lua_getglobal(L, "package");
> >  	int top = lua_gettop(L);
> >  
> > -- 
> > 2.21.0
> > 

Diff from my previous commit:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f96709a75..bb7405e68 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -758,15 +758,14 @@ 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)
-					break;
 				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;
 				}
 			}
 			/*
diff --git a/src/box/xrow.c b/src/box/xrow.c
index ccdd9def5..22a5be318 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -510,13 +510,11 @@ iproto_write_error(int fd, const struct error *e, uint32_t schema_version,
 
 	body.v_data_len = mp_bswap_u32(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;
+	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
@@ -1086,7 +1084,6 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot)
 		goto err;
 
 	assert(row->bodycnt == 1);
-
 	const char *data = start = (const char *) row->body[0].iov_base;
 	end = data + row->body[0].iov_len;
 	const char *tmp = data;
diff --git a/src/lua/init.c b/src/lua/init.c
index f66a3eb46..be164bdfb 100644
--- a/src/lua/init.c
+++ b/src/lua/init.c
@@ -318,10 +318,6 @@ static void
 tarantool_lua_setpaths(struct lua_State *L)
 {
 	const char *home = getenv("HOME");
-	char cwd[PATH_MAX] = {'\0'};
-	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);