[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs
Alexander Turenko
alexander.turenko at tarantool.org
Sun Apr 28 08:40:33 MSK 2019
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);
More information about the Tarantool-patches
mailing list