[tarantool-patches] Re: [PATCH v1] Fix LTO in travis-ci

[Alexander Turenko <alexander.turenko@tarantool.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 <avkhatskevich@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),