Message ID | 5523FCA6.2080504@redhat.com |
---|---|
State | Committed |
Headers |
Received: (qmail 89236 invoked by alias); 7 Apr 2015 15:50:09 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 89224 invoked by uid 89); 7 Apr 2015 15:50:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Message-ID: <5523FCA6.2080504@redhat.com> Date: Tue, 07 Apr 2015 17:49:58 +0200 From: Florian Weimer <fweimer@redhat.com> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Stefan Liebler <stli@linux.vnet.ibm.com>, libc-alpha@sourceware.org Subject: Re: [PATCH 06/25] Add struct scratch_buffer and its internal helper functions References: <cover.1425285061.git.fweimer@redhat.com> <7a6fe503fb764beee3d5b89662d3bbf65242161c.1425285061.git.fweimer@redhat.com> <54F4BB15.7070409@cs.ucla.edu> <550C37F7.10504@redhat.com> <550C4AE0.60205@cs.ucla.edu> <55103BEA.7070305@redhat.com> <5510505B.8060209@cs.ucla.edu> <alpine.DEB.2.10.1503231839380.14930@digraph.polyomino.org.uk> <55105F2C.6040400@redhat.com> <alpine.DEB.2.10.1503231845410.14930@digraph.polyomino.org.uk> <55105FED.80004@redhat.com> <551C1C9B.1060807@redhat.com> <551D8AE1.5000805@redhat.com> <551DE8EA.6080200@cs.ucla.edu> <5522BC03.5090300@redhat.com> <5523007D.9080906@cs.ucla.edu> <55239F13.5040700@redhat.com> <mg0lfr$ovf$1@ger.gmane.org> <5523EABF.9050003@redhat.com> In-Reply-To: <5523EABF.9050003@redhat.com> Content-Type: multipart/mixed; boundary="------------060500020405060102060500" |
Commit Message
Florian Weimer
April 7, 2015, 3:49 p.m. UTC
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.
Comments
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.
From 72301304a5655662baf2bae88a7aceeabc4a753e Mon Sep 17 00:00:00 2001 From: Florian Weimer <fweimer@redhat.com> Date: Tue, 7 Apr 2015 17:46:58 +0200 Subject: [PATCH] scratch_buffer_grow_preserve: Add missing #include <string.h> --- ChangeLog | 4 ++++ malloc/scratch_buffer_grow_preserve.c | 1 + 2 files changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index 4e1df07..c0d3aca 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 2015-04-07 Florian Weimer <fweimer@redhat.com> + * malloc/scratch_buffer_grow_preserve.c: Include <string.h> + +2015-04-07 Florian Weimer <fweimer@redhat.com> + * include/scratch_buffer.h: New file. * malloc/scratch_buffer_grow.c: Likewise. * malloc/scratch_buffer_grow_preserve.c: Likewise. diff --git a/malloc/scratch_buffer_grow_preserve.c b/malloc/scratch_buffer_grow_preserve.c index f2edb49..c5f0b2d 100644 --- a/malloc/scratch_buffer_grow_preserve.c +++ b/malloc/scratch_buffer_grow_preserve.c @@ -18,6 +18,7 @@ #include <scratch_buffer.h> #include <errno.h> +#include <string.h> bool __libc_scratch_buffer_grow_preserve (struct scratch_buffer *buffer) -- 2.1.0