[tarantool-patches] Re: [PATCH v1] build: LTO release build fails

[Alexander Turenko]

On Sat, Sep 21, 2019 at 01:02:09AM +0200, Vladislav Shpilevoy wrote:
> Hi! I am ok with the patch. What I don't understand is
> why it was working earlier, before my patches in the master.
>
> The problematic variables always were declared without an
> initial value, I didn't touch that part. But it was building
> ok. What has changed? Me and Alexander T. have failed to find
> a reason (although I didn't try super hard :) ).

I looked a bit into the problem. I'll describe my investigations below
the email for ones who interested. But in brief I have three ways to
solve the problem with those warnings on LTO build:

1. Initialize the variables on which GCC complains.
2. Set -Wno-error=maybe-uninitialized for GCC (Clang has no such
   option).
3. Add __attribute__((always_inline)) for update_err_arg_type().

I don't want to do the second, because warnings will be shown and that
looks ugly. I also don't want to suppress the warning with
-Wno-maybe-uninitialized, because it sometimes gives useful information.

Note for 3rd variant: I didn't look deeply into the patch where the
function was added. Just adding the attribute to update_err_arg_type()
works for me, but maybe other functions needs it too to be strong
against inlining changes if we'll going that way.

Another note for 3rd: We need to verify that the attribute is supported
by all compilers we support (gcc and clang of different versions) if
we'll going this way. Or determine whether it is supported and add only
in the case: it seems zstd doing something like that.

I don't sure what is better: 1st or 3rd. I would let author of the new
code (Vlad via bae634bb7a) decide.

WBR, Alexander Turenko.

----

First, look into the discussion:
https://gcc.gnu.org/bugzilla//show_bug.cgi?id=60165

In brief, I understood it so.

It looks for code paths like the following within a function:

int foo()
{
	int x;
	if (cond)
		x = 42;
	use x;
	...
}

It does not warn in cases like the following, because the pass is local
for a function and f(&x) is considered as an assignment (this is just
heuristics):

int f()
{
	if (cond) {
		x = 42;
		return 0;
	}
	return -1;
}

int bar()
{
	int x;
	f(&x);
	use x;
	...
}

The same for the following (no warning):

int f()
{
	if (cond) {
		x = 42;
		return 0;
	}
	return -1;
}

int baz()
{
	int x;
	if (f(&x))
		return -1;
	use x;
	...
}

However it should warn for such code (with inlined f()):

void feez()
{
	int x;
	int f_rc;
	if (cond) {
		x = 42;
		f_rc = 0;
	} else {
		f_rc = -1;
	}
	if (f_rc != 0)
		return -1;
	use x;
	...
}

The main point here that the check is performed after inlining.

I experimenting a bit with code like above, but don't get a warning
where expected. It seems that there are some other heuristics in the
compiler that I don't know about. But I hope that the general logic is
right.

Now let's come back to our case. I run the build w/o parallelization
(just make, w/o -j) on the current master (4c2d1eff) and observed that
the warning re 'maybe uninitialized' str_len in do_op_splice() don't
produce for src/tarantool executable, but produced for
test/unit/tuple_bigref.test executable (during linking).

I compared resulting code (don't know how to get IR from LTO stage for
GCC) and observed that a version of do_op_splice() in src/tarantool
inlines update_err_arg_type(), while do_op_splice() in the test contains
calls to update_err_arg_type().

do_op_splice() from the test contains the following snippet:

665a7: callq  62bb0 <update_err_arg_type>
665ac: test   %eax,%eax
665ae: jne    664fe <do_op_splice+0x1ae>
<...>
664fe: lea    -0x20(%rbp),%rsp
66502: mov    $0xffffffff,%eax
66507: pop    %rbx
66508: pop    %r12
6650a: pop    %r13
6650c: pop    %r14
6650e: pop    %rbp
6650f: retq

It seems that usage of str_len is somewhere in <...> and the compiler
cannot prove that we'll return from the function right after call to
update_err_arg_type(). So there are possible (in theory) code paths
where we use uninitialized str_len value.

A call to update_err_arg_type() does not use &str_len and so 'it is like
assignment' heuristics does not work here as it would be with a
non-inlined mp_read_str() call.

After that I added __attribute__((always_inline)) to
update_err_arg_type() and that take all warnings away.