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 F04CD2F0D2 for ; Thu, 16 May 2019 12:22:22 -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 MWaBI1W9lt7R for ; Thu, 16 May 2019 12:22:22 -0400 (EDT) Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 1EC672F0D1 for ; Thu, 16 May 2019 12:22:21 -0400 (EDT) Date: Thu, 16 May 2019 19:22:03 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v1] Fix LTO in travis-ci Message-ID: <20190516162203.xcahozsadckwcvfg@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit 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: "Alexander V. Tikhonov" Cc: tarantool-patches@freelists.org > Fix LTO in travis-ci Need to mention that clang is fixed too. BTW, I usually use prefix 'travis-ci:' for such commits. > > Made fixes: > > - Added CMAKE_EXTRA_PARAMS environment to docker's container > runs to enable -DENABLE_LTO=ON/OFF cmake option. > > - Added CC/CXX environment to docker's container runs to set > clang for cmake. > > - Changed LTO docker's image to 'debian-buster' due to LTO needed > higher versions of packages, check for more information commit: > > commit f9e28ce4602aff3f9bb4e743b0d6167b0f8df88d > Author: AKhatskevich > Date: Thu Mar 22 11:37:06 2018 +0300 We usually reference other commits like 0000000000000000000000000000000000000000 ('title of the commit here'). > > - Fixed sources to avoid of failures on builds with LTO: Is it about clang or gcc? Which version? I need to know to reproduce. > > 1) src/box/memtx_rtree.c: In function ‘mp_decode_rect’: > src/box/memtx_rtree.c:86:24: error: ‘c’ may be used uninitialized > in this function [-Werror=maybe-uninitialized] > rect->coords[i * 2] = c; > ^ > src/box/memtx_rtree.c:74:10: note: ‘c’ was declared here > coord_t c; > ^ > > 2) src/box/sql/func.c: In function ‘quoteFunc’: > src/box/sql/func.c:1103:3: error: ‘b’ may be used uninitialized > in this function [-Werror=maybe-uninitialized] > sql_result_text(context, sql_value_boolean(argv[0]) ? > ^ > src/box/sql/vdbeapi.c:217:7: note: ‘b’ was declared here > bool b; > ^ > > 3) src/box/tuple_update.c: In function ‘update_read_ops’: > src/box/tuple_update.c:1022:4: error: ‘field_no’ may be used > uninitialized in this function [-Werror=maybe-uninitialized] > diag_set(ClientError, ER_NO_SUCH_FIELD_NO, field_no); > ^ > src/box/tuple_update.c:1014:11: note: ‘field_no’ was declared here > int32_t field_no; > ^ > > 4) src/httpc.c: In function ‘httpc_set_verbose’: > src/httpc.c:267:2: error: call to ‘_curl_easy_setopt_err_long’ > declared with attribute warning: curl_easy_setopt expects a long > argument for this option [-Werror] > curl_easy_setopt(req->curl_request.easy, CURLOPT_VERBOSE, curl_verbose); > ^ > > 5) src/lua/httpc.c: In function ‘luaT_httpc_request’: > src/lua/httpc.c:128:64: error: ‘MEM[(int *)&parser + 20B]’ may be used > uninitialized in this function [-Werror=maybe-uninitialized] > lua_pushinteger(L, (parser.http_minor > 0) ? parser.http_minor: 0); > ^ > src/lua/httpc.c:67:21: note: ‘MEM[(int *)&parser + 20B]’ was declared here > struct http_parser parser; > ^ > src/lua/httpc.c:124:64: error: ‘MEM[(int *)&parser + 16B]’ may be used > uninitialized in this function [-Werror=maybe-uninitialized] > lua_pushinteger(L, (parser.http_major > 0) ? parser.http_major: 0); > ^ > src/lua/httpc.c:67:21: note: ‘MEM[(int *)&parser + 16B]’ was declared here > struct http_parser parser; > ^ > > Close #4215 > --- > > Github: https://github.com/tarantool/tarantool/compare/avtikhon/lto-build-fix > Issue: https://github.com/tarantool/tarantool/issues/4215 > > .travis.mk | 7 +++++-- > .travis.yml | 4 ++-- > src/box/memtx_rtree.c | 2 +- > src/box/sql/vdbeapi.c | 2 +- > src/box/tuple_update.c | 2 +- > src/httpc.c | 2 +- > src/lua/httpc.c | 34 ++++++++++++++++++---------------- > 7 files changed, 29 insertions(+), 24 deletions(-) > > diff --git a/.travis.mk b/.travis.mk > index 4ac3fc11a..55bee9980 100644 > --- a/.travis.mk > +++ b/.travis.mk > @@ -2,7 +2,7 @@ > # Travis CI rules > # > > -DOCKER_IMAGE:=packpack/packpack:debian-stretch > +DOCKER_IMAGE?=packpack/packpack:debian-stretch > > all: package > > @@ -27,6 +27,9 @@ docker_%: > -e CCACHE_DIR=/cache/ccache \ > -e COVERALLS_TOKEN=${COVERALLS_TOKEN} \ > -e TRAVIS_JOB_ID=${TRAVIS_JOB_ID} \ > + -e CMAKE_EXTRA_PARAMS=${CMAKE_EXTRA_PARAMS} \ > + -e CC=${CC} \ > + -e CXX=${CXX} \ It is a kind of nitpicking, but formally Travis-CI sets also {CC,CXX}_FOR_BUILD. I would add the link [1] to the commit message and clarify that we don't needed {CC,CXX}_FOR_BUILD, because don't doing cross-compilation at the moment. This could help if we'll add it somewhere in the future. [1]: https://docs.travis-ci.com/user/languages/cpp/#choosing-compilers-to-test-against > ${DOCKER_IMAGE} \ > make -f .travis.mk $(subst docker_,,$@) > > @@ -37,7 +40,7 @@ deps_ubuntu: > libcurl4-openssl-dev libunwind-dev libicu-dev \ > python python-pip python-setuptools python-dev \ > python-msgpack python-yaml python-argparse python-six python-gevent \ > - lcov ruby > + lcov ruby clang llvm llvm-dev Whether our separate deps_clang_8 rule (that is planned to enable on gitlab ci) will work good after clang/llvm/llvm-dev installing here? I would think whether we can add separate deps_clang rule in the similar way and call it from .travis.yml (or from ubuntu_clang target in .travis.mk?). If there is a good way to split it out, then do. If not, I don't insist on that. > > test_ubuntu: deps_ubuntu > cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS} > diff --git a/.travis.yml b/.travis.yml > index 6e79cf2fe..92fb54686 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -37,10 +37,10 @@ jobs: > if: branch = "master" > # Special targets (only LTO for now). > - name: "LTO build + test (Linux, gcc)" > - env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON > + env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON DOCKER_IMAGE=packpack/packpack:debian-buster I would break the line: | # Special targets (only LTO for now). | - name: "LTO build + test (Linux, gcc)" | env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON | DOCKER_IMAGE=packpack/packpack:debian-buster | if: branch = "master" It is valid yaml, see 'plain style' here: https://stackoverflow.com/a/21699210 Or check youself with, say, tarantool yaml: | unpack(require('yaml').decode(io.open('.travis.yml'):read(1000000)).jobs.include) > if: branch = "master" > - name: "LTO build + test (Linux, clang)" > - env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON > + env: TARGET=test CMAKE_EXTRA_PARAMS=-DENABLE_LTO=ON DOCKER_IMAGE=packpack/packpack:debian-buster > if: branch = "master" > compiler: clang > - name: "LTO build + test (OS X Mojave 10.14)" > diff --git a/src/box/memtx_rtree.c b/src/box/memtx_rtree.c > index 45e8fb8e3..b91a405d7 100644 > --- a/src/box/memtx_rtree.c > +++ b/src/box/memtx_rtree.c > @@ -71,7 +71,7 @@ static inline int > mp_decode_rect(struct rtree_rect *rect, unsigned dimension, > const char *mp, unsigned count, const char *what) > { > - coord_t c; > + coord_t c = 0; > if (count == dimension) { /* point */ > for (unsigned i = 0; i < dimension; i++) { > if (mp_decode_num(&mp, i, &c) < 0) Ok. > diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c > index d2868567b..04083914a 100644 > --- a/src/box/sql/vdbeapi.c > +++ b/src/box/sql/vdbeapi.c > @@ -214,7 +214,7 @@ sql_value_double(sql_value * pVal) > bool > sql_value_boolean(sql_value *val) > { > - bool b; > + bool b = false; > mem_value_bool((struct Mem *) val, &b); > return b; > } I would also update sql_value_double(), sql_value_int(), sql_value_int64() to keep them consistent. It also worth to explicitly express that we sure that the mem value has needed type, like so: | bool b = false; | int rc = mem_value_bool(..., &b); | assert(rc == 0); | (void) rc; | return b; > diff --git a/src/box/tuple_update.c b/src/box/tuple_update.c > index 849073258..7a203ced8 100644 > --- a/src/box/tuple_update.c > +++ b/src/box/tuple_update.c > @@ -1011,7 +1011,7 @@ update_read_ops(struct tuple_update *update, const char *expr, > "field id must be a number"); > return -1; > } > - int32_t field_no; > + int32_t field_no = 0; > if (mp_read_i32(update->index_base, op, &expr, &field_no)) > return -1; > if (field_no - update->index_base >= 0) { Ok. > diff --git a/src/httpc.c b/src/httpc.c > index 80dabd59c..813a3e387 100644 > --- a/src/httpc.c > +++ b/src/httpc.c > @@ -267,7 +267,7 @@ httpc_set_low_speed_limit(struct httpc_request *req, long low_speed_limit) > void > httpc_set_verbose(struct httpc_request *req, bool curl_verbose) > { > - curl_easy_setopt(req->curl_request.easy, CURLOPT_VERBOSE, curl_verbose); > + curl_easy_setopt(req->curl_request.easy, CURLOPT_VERBOSE, (long) curl_verbose); Please, keep lines within 80 symbols. > } > > void > diff --git a/src/lua/httpc.c b/src/lua/httpc.c > index c39a2f0de..c174ffce0 100644 > --- a/src/lua/httpc.c > +++ b/src/lua/httpc.c > @@ -64,9 +64,10 @@ static int > parse_headers(lua_State *L, char *buffer, size_t len, > int max_header_name_len) > { > - struct http_parser parser; > - parser.hdr_name = (char *) calloc(max_header_name_len, sizeof(char)); > - if (parser.hdr_name == NULL) { > + struct http_parser *parser = (struct http_parser *) > + malloc(sizeof(*parser)); > + parser->hdr_name = (char *) calloc(max_header_name_len, sizeof(char)); > + if (parser->hdr_name == NULL) { > diag_set(OutOfMemory, max_header_name_len * sizeof(char), > "malloc", "hdr_name"); > return -1; It seems dynamic allocation just prevents the warning from raising. I would fix it in more straight way (see below). I didn't check this patch against warnings with LTO, but it should help. diff --git a/src/lib/http_parser/http_parser.c b/src/lib/http_parser/http_parser.c index b9490117e..d0cba0d12 100644 --- a/src/lib/http_parser/http_parser.c +++ b/src/lib/http_parser/http_parser.c @@ -40,6 +40,16 @@ * adaptation from nginx http parser module */ +void http_parser_create(struct http_parser *parser) +{ + parser->hdr_value_start = NULL; + parser->hdr_value_end = NULL; + parser->http_major = -1; + parser->http_minor = -1; + parser->hdr_name = NULL; + parser->hdr_name_idx = 0; +} + /** * Utility function used in headers parsing */ diff --git a/src/lib/http_parser/http_parser.h b/src/lib/http_parser/http_parser.h index 9d6cc0932..16c083b43 100644 --- a/src/lib/http_parser/http_parser.h +++ b/src/lib/http_parser/http_parser.h @@ -49,7 +49,13 @@ struct http_parser { int hdr_name_idx; }; -/* +/** + * @brief Initialize an http parser. + * @param parser structure to initialize + */ +void http_parser_create(struct http_parser *parser); + +/** * @brief Parse line containing http header info * @param parser object * @param bufp pointer to buffer with data diff --git a/src/lua/httpc.c b/src/lua/httpc.c index d3d80bfeb..bc8d498ba 100644 --- a/src/lua/httpc.c +++ b/src/lua/httpc.c @@ -65,6 +65,7 @@ parse_headers(lua_State *L, char *buffer, size_t len, int max_header_name_len) { struct http_parser parser; + http_parser_create(&parser); parser.hdr_name = (char *) calloc(max_header_name_len, sizeof(char)); if (parser.hdr_name == NULL) { diag_set(OutOfMemory, max_header_name_len * sizeof(char),