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

[Vladislav Shpilevoy]

Thanks for the investigation. As I understood, my changes allowed the
compiler to inline the warned functions somehow differently, after
what he noticed the 'problem'.

I think, we can just initialize them. Always_inline thing looks too
complex. We use compiler hints like likely/unlikely/prefetch/always_inline,
etc only in exceptional cases, to which the being discussed one does
not belong, I guess.

In other words, the initial patch still LGTM.

On 23/09/2019 03:06, Alexander Turenko wrote:
> 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.
>