[06/25] Add struct scratch_buffer and its internal helper functions

  On 04/07/2015 04:33 PM, Florian Weimer wrote:
> On 04/07/2015 03:18 PM, Stefan Liebler wrote:
>> Hi Florian,
>>
>> i get a build error on s390x:
>> gcc scratch_buffer_grow_preserve.c -c
>> ...
>> scratch_buffer_grow_preserve.c: In function
>> ‘__libc_scratch_buffer_grow_preserve’:
>> scratch_buffer_grow_preserve.c:35:7: error: implicit declaration of
>> function ‘memcpy’ [-Werror=implicit-function-declaration]
>>        memcpy (new_ptr, buffer->__space, buffer->length);
>>        ^
>> scratch_buffer_grow_preserve.c:35:7: error: incompatible implicit
>> declaration of built-in function ‘memcpy’ [-Werror]
>> cc1: all warnings being treated as errors
>>
>> #include <string.h> in malloc/scratch_buffer_grow_preserve.c solves this
>> error. Please update scratch_buffer_grow_preserve.c.
> 
> Sorry about that.  I did build on ppc64, but building on s390x is
> difficult for me due to the binutils 2.24 requirement.  I'm trying to
> get a suitable machine reservation, but I'm not sure if that will succeed.
> 
> Should I commit this fix blindly, without testing it on s390x?

I found a machine to test this on, so I committed the attached patch.
  

On 04/07/2015 05:49 PM, Florian Weimer wrote:
> On 04/07/2015 04:33 PM, Florian Weimer wrote:
>> On 04/07/2015 03:18 PM, Stefan Liebler wrote:
>>> Hi Florian,
>>>
>>> i get a build error on s390x:
>>> gcc scratch_buffer_grow_preserve.c -c
>>> ...
>>> scratch_buffer_grow_preserve.c: In function
>>> ‘__libc_scratch_buffer_grow_preserve’:
>>> scratch_buffer_grow_preserve.c:35:7: error: implicit declaration of
>>> function ‘memcpy’ [-Werror=implicit-function-declaration]
>>>         memcpy (new_ptr, buffer->__space, buffer->length);
>>>         ^
>>> scratch_buffer_grow_preserve.c:35:7: error: incompatible implicit
>>> declaration of built-in function ‘memcpy’ [-Werror]
>>> cc1: all warnings being treated as errors
>>>
>>> #include <string.h> in malloc/scratch_buffer_grow_preserve.c solves this
>>> error. Please update scratch_buffer_grow_preserve.c.
>>
>> Sorry about that.  I did build on ppc64, but building on s390x is
>> difficult for me due to the binutils 2.24 requirement.  I'm trying to
>> get a suitable machine reservation, but I'm not sure if that will succeed.
>>
>> Should I commit this fix blindly, without testing it on s390x?
>
> I found a machine to test this on, so I committed the attached patch.
>
Thanks.

I have another issue with tst-scratch_buffer.c on s390-32 where size_t 
is an unsigned long with only 4 bytes:
gcc tst-scratch_buffer.c -c
...
tst-scratch_buffer.c: In function ‘do_test’:
tst-scratch_buffer.c:133:8: error: large integer implicitly truncated to 
unsigned type [-Werror=overflow]
         && unchanged_array_size (&buf, 1ULL << 32, 0)
         ^
tst-scratch_buffer.c:134:8: error: large integer implicitly truncated to 
unsigned type [-Werror=overflow]
         && unchanged_array_size (&buf, 0, 1ULL << 32)))
         ^
cc1: all warnings being treated as errors

Can you change the test and use "1ULL << 31" or make the usage of "1ULL 
<< 32" conditionally?

Bye Stefan
  

On 04/09/2015 12:18 PM, Stefan Liebler wrote:

> I have another issue with tst-scratch_buffer.c on s390-32 where size_t
> is an unsigned long with only 4 bytes:

Dave Miller reported that as well, see the parallel thread.

I need to figure out how to build glibc for 32-bit in a multi-arch
environment.  But 32-bit s390 could be tricky, again due to the binutils
2.24 environment.

> gcc tst-scratch_buffer.c -c
> ...
> tst-scratch_buffer.c: In function ‘do_test’:
> tst-scratch_buffer.c:133:8: error: large integer implicitly truncated to
> unsigned type [-Werror=overflow]
>         && unchanged_array_size (&buf, 1ULL << 32, 0)
>         ^
> tst-scratch_buffer.c:134:8: error: large integer implicitly truncated to
> unsigned type [-Werror=overflow]
>         && unchanged_array_size (&buf, 0, 1ULL << 32)))
>         ^
> cc1: all warnings being treated as errors
> 
> Can you change the test and use "1ULL << 31" or make the usage of "1ULL
> << 32" conditionally?

I have a patch that adds a cast to size_t, which suppresses the warning
on 32-bit platforms.

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?
  

On 04/09/2015 01:07 PM, Florian Weimer wrote:
> On 04/09/2015 12:18 PM, Stefan Liebler wrote:
>
>> I have another issue with tst-scratch_buffer.c on s390-32 where size_t
>> is an unsigned long with only 4 bytes:
>
> Dave Miller reported that as well, see the parallel thread.
>
> I need to figure out how to build glibc for 32-bit in a multi-arch
> environment.  But 32-bit s390 could be tricky, again due to the binutils
> 2.24 environment.
You can use a chroot with installed 32bit-packages and start a bash with 
linux32.
But you are right, you need at least binutils 2.24.

>
>> gcc tst-scratch_buffer.c -c
>> ...
>> tst-scratch_buffer.c: In function ‘do_test’:
>> tst-scratch_buffer.c:133:8: error: large integer implicitly truncated to
>> unsigned type [-Werror=overflow]
>>          && unchanged_array_size (&buf, 1ULL << 32, 0)
>>          ^
>> tst-scratch_buffer.c:134:8: error: large integer implicitly truncated to
>> unsigned type [-Werror=overflow]
>>          && unchanged_array_size (&buf, 0, 1ULL << 32)))
>>          ^
>> cc1: all warnings being treated as errors
>>
>> Can you change the test and use "1ULL << 31" or make the usage of "1ULL
>> << 32" conditionally?
>
> 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).


>
> 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]
In file included from tst-scratch_buffer.c:19:0:
../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:93:25: error: called from here [-Werror=inline]
In file included from tst-scratch_buffer.c:19:0:
../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:119:25: error: called from here [-Werror=inline]
In file included from tst-scratch_buffer.c:19:0:
../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:141:25: error: called from here [-Werror=inline]


Bye
Stefan
  

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?
  

From: Florian Weimer <fweimer@redhat.com>

Date: Thu, 09 Apr 2015 17:09:54 +0200

> 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.


These are the same warnings I get on sparc, FWIW.

> 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?


I agree that -Winline should be removed.
  

Heretofore we have said that the inline keyword should be dropped if it's
not essential that something be inlined.  Then the compiler will still
inline it if it thinks that's optimal, but won't warn.
  

From: Roland McGrath <roland@hack.frob.com>
Date: Thu,  9 Apr 2015 12:24:08 -0700 (PDT)

> Heretofore we have said that the inline keyword should be dropped if it's
> not essential that something be inlined.  Then the compiler will still
> inline it if it thinks that's optimal, but won't warn.

In this particular case the inline is in a header file to provide a
function used by applications.

If you remove the inline keyword, then every include of scratch_buffer.h
gets a static copy in it's object file, moreso the compiler is going to
warn if the function is unused by that foo.c file.

I still stand by my position that -Winline is foolhardy and thus should
be removed.
  

David Miller wrote:
> In this particular case the inline is in a header file to provide a
> function used by applications.

As I understand it <scratch_buffer.h> is a private header, not for use by 
applications.  See, e.g., 
<https://sourceware.org/ml/libc-alpha/2015-04/msg00047.html> where I ask "These 
functions are all private to glibc, right?" and the followup 
<https://sourceware.org/ml/libc-alpha/2015-04/msg00067.html> where Florian says 
"My concern was the internal ABI between libc and the NSS modules."

If <scratch_buffer.h> is intended to be public then we have a problem, as C99 
frowns on public headers defining static inline functions for use in application 
code.
  

On 04/10/2015 08:56 AM, Paul Eggert wrote:
> David Miller wrote:
>> In this particular case the inline is in a header file to provide a
>> function used by applications.
> 
> As I understand it <scratch_buffer.h> is a private header, not for use
> by applications.  See, e.g.,
> <https://sourceware.org/ml/libc-alpha/2015-04/msg00047.html> where I ask
> "These functions are all private to glibc, right?" and the followup
> <https://sourceware.org/ml/libc-alpha/2015-04/msg00067.html> where
> Florian says "My concern was the internal ABI between libc and the NSS
> modules."
> 
> If <scratch_buffer.h> is intended to be public then we have a problem,
> as C99 frowns on public headers defining static inline functions for use
> in application code.

It's not intended as a public header, not for the foreseeable future.
The concern about unused static functions is still valid, though.

I think we either have to ban “inline” and use “__always_inline”
exclusively where that makes sense, or drop -Winline.  As I said, the
latter shouldn't be needed anymore because __always_inline failures are
covered under -Wattributes.
  

On Fri, Apr 10, 2015 at 10:01:34AM +0200, Florian Weimer wrote:
> On 04/10/2015 08:56 AM, Paul Eggert wrote:
> > David Miller wrote:
> >> In this particular case the inline is in a header file to provide a
> >> function used by applications.
> > 
> > As I understand it <scratch_buffer.h> is a private header, not for use
> > by applications.  See, e.g.,
> > <https://sourceware.org/ml/libc-alpha/2015-04/msg00047.html> where I ask
> > "These functions are all private to glibc, right?" and the followup
> > <https://sourceware.org/ml/libc-alpha/2015-04/msg00067.html> where
> > Florian says "My concern was the internal ABI between libc and the NSS
> > modules."
> > 
> > If <scratch_buffer.h> is intended to be public then we have a problem,
> > as C99 frowns on public headers defining static inline functions for use
> > in application code.
> 
> It's not intended as a public header, not for the foreseeable future.
> The concern about unused static functions is still valid, though.
> 
> I think we either have to ban “inline” and use “__always_inline”
> exclusively where that makes sense, or drop -Winline.  As I said, the
> latter shouldn't be needed anymore because __always_inline failures are
> covered under -Wattributes.
> 
Agreed. Its known problem that gcc cannot handle macro-like inline
functions well so forcing always_inline is appropriate. gcc uses
heuristic that use function size as parameter regardless that it could
be almost optimized out when inlined.