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

[Alexander Turenko]

Okay.

Pushed to master.

WBR, Alexander Turenko.

On Mon, Sep 23, 2019 at 09:36:37PM +0200, Vladislav Shpilevoy wrote:
> 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.
> >