-Winline option (was: Re: [PATCH 06/25] Add struct scratch_buffer and its internal helper functions)

[Florian Weimer <fweimer at redhat dot com>]

On 04/09/2015 04:17 PM, Stefan Liebler wrote:

>> I have a patch that adds a cast to size_t, which suppresses the warning
>> on 32-bit platforms.
> Yes, "(size_t) (1ULL << 32)" suppresses the warning (tested with gcc
> 4.6.4, 4.7.4, 4.8.4, 4.9.2).

Okay, I will commit that shortly.

>> Using 1ULL << 31 would invalidate the test.
>>
>> Can you make a change to the test, so that it compiles, and check if you
>> get the same inlining failure as Dave?
>>
> I get the following inlining failures with gcc 4.6.4/4.7.4 on 32/64bit:
> In file included from tst-scratch_buffer.c:19:0:
> tst-scratch_buffer.c: In function âdo_testâ:
> ../include/scratch_buffer.h:85:1: error: inlining failed in call to
> âscratch_buffer_freeâ: call is unlikely and code size would grow
> [-Werror=inline]
> tst-scratch_buffer.c:79:25: error: called from here [-Werror=inline]

If the line numbers are correct, that's the start of do_test:

    struct scratch_buffer buf;
    scratch_buffer_init (&buf);
    memset (buf.data, ' ', buf.length);
    scratch_buffer_free (&buf);

And I don't see why this code path could be assumed to be unlikely.  It
might be a GCC bug.  I don't see it with later versions.

But the fact is that we want to use inline functions on unlikely paths,
and there's nothing wrong with not inlining them there.  I don't know
why we activate -Winline.  The option was apparently added to glibc when
GCC did not support the always_inline attribute.  If we need inlining
for correctness, we should just define the function with
__always_inline.  This way, inlining failures will receive a warning
under -Wattributes, which is enabled by default.  We can then drop
-Winline and let GCC do what it thinks is best.

Comments?

-- 
Florian Weimer / Red Hat Product Security