[tarantool-patches] Re: [PATCH v1] Fix LTO in travis-ci
Alexander Turenko
alexander.turenko at tarantool.org
Thu May 16 19:22:03 MSK 2019
> 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 <avkhatskevich at tarantool.org>
> 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),
More information about the Tarantool-patches
mailing list