From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E69832D205 for ; Sun, 28 Apr 2019 01:40:47 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ROkLCHwdriDS for ; Sun, 28 Apr 2019 01:40:47 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 74DE72CD77 for ; Sun, 28 Apr 2019 01:40:46 -0400 (EDT) Date: Sun, 28 Apr 2019 08:40:33 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs Message-ID: <20190428054028.vxdwsboswt5strjl@tkn_work_nb> References: <64db98703d236c2e6775601a48fb19c3bc8ba955.1556241512.git.alexander.turenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.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);