Message ID | oregrq414g.fsf@free.home |
---|---|
State | New, archived |
Headers |
Received: (qmail 18257 invoked by alias); 23 Dec 2014 19:28:32 -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 18235 invoked by uid 89); 23 Dec 2014 19:28:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com From: Alexandre Oliva <aoliva@redhat.com> To: libc-alpha@sourceware.org Subject: Re: fix -Werror regressions with gcc 4.8 on x86_64 References: <orr3vr3pa1.fsf@free.home> Date: Tue, 23 Dec 2014 17:28:15 -0200 In-Reply-To: <orr3vr3pa1.fsf@free.home> (Alexandre Oliva's message of "Tue, 23 Dec 2014 03:31:50 -0200") Message-ID: <oregrq414g.fsf@free.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii |
Commit Message
Alexandre Oliva
Dec. 23, 2014, 7:28 p.m. UTC
On Dec 23, 2014, Alexandre Oliva <aoliva@redhat.com> wrote: > Here's a patch that fixes a few build errors I got when using GCC 4.8 > with -Werror. Ok to install? And here are a few more that I only hit when running the testsuite... Ok to install? for ChangeLog * dlfcn/bug-dl-leaf-lib (lib_main): Mark ret as unused. * math/atest-exp.c (exp_mpn): Likewise chk. * math/atest-exp2.c (exp_mpn): Likewise. --- dlfcn/bug-dl-leaf-lib.c | 2 +- math/atest-exp.c | 2 +- math/atest-exp2.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
Comments
On Tue, 23 Dec 2014, Alexandre Oliva wrote: > On Dec 23, 2014, Alexandre Oliva <aoliva@redhat.com> wrote: > > > Here's a patch that fixes a few build errors I got when using GCC 4.8 > > with -Werror. Ok to install? > > And here are a few more that I only hit when running the testsuite... > Ok to install? Are you building with -DNDEBUG? That's the only way I can see these variables being unused (set-but-not-used). NDEBUG is definitely a broken configuration for running the testsuite, as it means tests aren't checking what they should if the verification uses assert - the warnings are showing up a problem with your environment, not with glibc (see what Roland said in <https://sourceware.org/ml/libc-alpha/2014-12/msg00589.html>). And I'm doubtful NDEBUG is a sensible configuration for building glibc itself either; at least, it's not a well-tested one. Thus, this patch seems wrong, and I'm dubious of the previous patch without a clean consensus to support NDEBUG builds of glibc.
On 12/23/2014 2:56 PM, Joseph Myers wrote: > Are you building with -DNDEBUG? That's the only way I can see these > variables being unused (set-but-not-used). NDEBUG is definitely a broken > configuration for running the testsuite, as it means tests aren't checking > what they should if the verification uses assert - the warnings are > showing up a problem with your environment, not with glibc (see what > Roland said in > <https://sourceware.org/ml/libc-alpha/2014-12/msg00589.html>). And I'm > doubtful NDEBUG is a sensible configuration for building glibc itself > either; at least, it's not a well-tested one. > > Thus, this patch seems wrong, and I'm dubious of the previous patch > without a clean consensus to support NDEBUG builds of glibc. NDEBUG is added by default to RedHat/Fedora builds (other than prereleases). This has been true since 2009; Andreas can perhaps comment on this, since the changelog shows he enabled it.
Chris Metcalf <cmetcalf@ezchip.com> writes: > NDEBUG is added by default to RedHat/Fedora builds (other than prereleases). > This has been true since 2009; Andreas can perhaps comment on this, since the > changelog shows he enabled it. The fedora glibc has always been built with NDEBUG. My change was to disable it for glibc pre-releases, for better test coverage. Andreas.
On Dec 23, 2014, Joseph Myers <joseph@codesourcery.com> wrote: > On Tue, 23 Dec 2014, Alexandre Oliva wrote: >> On Dec 23, 2014, Alexandre Oliva <aoliva@redhat.com> wrote: >> >> > Here's a patch that fixes a few build errors I got when using GCC 4.8 >> > with -Werror. Ok to install? >> >> And here are a few more that I only hit when running the testsuite... >> Ok to install? > Are you building with -DNDEBUG? Yeah. I had no idea I was, but it looks like I picked that (and -O3) up from a config.status created by a Fedora or Red Hat Enterprise [GNU/]Linux build. Taking them out, the build failures have indeed gone away. Now, if we want to rule out -DNDEBUG builds, shouldn't we have some Makefile CFLAGS/CPPFLAGS tester to report this error, instead of failing in such obscure ways, that is a sure way to invite others to report and offer patches in the future, in addition to requiring Red Hat and Fedora to carry patches in future builds if they decide to keep on building glibc with -DNDEBUG?
I think what we want is to support building libc with NDEBUG but ensure that this doesn't break the test suite. One approach is some makefile machinations to ensure that -DNDEBUG gets removed from the options when compiling the test sources. But I think the better approach is to change all the test code using assert to use a different macro that exists just for test code, and never elides the checks.
On 01/05/2015 03:39 PM, Roland McGrath wrote: > I think what we want is to support building libc with NDEBUG but ensure > that this doesn't break the test suite. One approach is some makefile > machinations to ensure that -DNDEBUG gets removed from the options when > compiling the test sources. But I think the better approach is to change > all the test code using assert to use a different macro that exists just > for test code, and never elides the checks. > For use within glibc itself, I think we should redefine assert with NDEBUG to use __builtin_unreachable. For the public header file, I guess we can't do that since we're not allowed to have the side effects of the evaluation of the expression. But that's always bad practice, and we shouldn't have any of that in glibc anyway. I do wonder about the public header file using an expression like __ASSERT_VOID_CAST (0 && (expr)) Side effects are not evaluated, but identifiers referenced in the expression still get marked as "used", etc. r~
On 01/06/2015 08:35 AM, Richard Henderson wrote: > I do wonder about the public header file using an expression like > > __ASSERT_VOID_CAST (0 && (expr)) > > Side effects are not evaluated, but identifiers referenced in the expression > still get marked as "used", etc. Just as a heads-up, the GNU Emacs internals have a macro 'eassert' that does that, and it works well; we prefer it to 'assert'. There's also a macro 'eassume' that uses __builtin_unreachable; we tried having eassert do that too, but it led to too many compile-time false-alarms. However, the C Standard doesn't allow eassert's implementation 'assert', as it requires that if NDEBUG is defined, then 'assert' must be defined via '#define assert(x) ((void)0)', which means weird uses like 'assert(***)' have to work. Perhaps the public header file could use '0 && (expr)' only when in standards-conforming mode?
> For use within glibc itself, I think we should redefine assert with NDEBUG to > use __builtin_unreachable. The point of NDEBUG is to optimize out the tests, not just to avoid the file name, line number, and expression source strings.
On 01/07/2015 03:19 PM, Roland McGrath wrote: >> For use within glibc itself, I think we should redefine assert with NDEBUG to >> use __builtin_unreachable. > > The point of NDEBUG is to optimize out the tests, not just to avoid the > file name, line number, and expression source strings. > The tests themselves can sometimes give information to the compiler. Showing it that certain paths are unreachable imply information for the other paths. The __builtin_unreachable function doesn't imply any actual effect; the paths leading to this function should be removed. r~
> On 01/07/2015 03:19 PM, Roland McGrath wrote: > >> For use within glibc itself, I think we should redefine assert with NDEBUG to > >> use __builtin_unreachable. > > > > The point of NDEBUG is to optimize out the tests, not just to avoid the > > file name, line number, and expression source strings. > > > > The tests themselves can sometimes give information to the compiler. Showing > it that certain paths are unreachable imply information for the other paths. > The __builtin_unreachable function doesn't imply any actual effect; the paths > leading to this function should be removed. OK.
diff --git a/dlfcn/bug-dl-leaf-lib.c b/dlfcn/bug-dl-leaf-lib.c index 4afd81b..e7b19f2 100644 --- a/dlfcn/bug-dl-leaf-lib.c +++ b/dlfcn/bug-dl-leaf-lib.c @@ -50,7 +50,7 @@ void check_val_fini (void) int lib_main (void) { - int ret; + int ret __attribute__((__unused__)); void *hdl; /* Make sure the constructor sees the updated val. */ diff --git a/math/atest-exp.c b/math/atest-exp.c index 406b00b..4eb1728 100644 --- a/math/atest-exp.c +++ b/math/atest-exp.c @@ -61,7 +61,7 @@ exp_mpn (mp1 ex, mp1 x) unsigned n; mp1 xp; mp2 tmp; - mp_limb_t chk; + mp_limb_t chk __attribute__((__unused__)); mp1 tol; memset (xp, 0, sizeof (mp1)); diff --git a/math/atest-exp2.c b/math/atest-exp2.c index 4599994..527377e 100644 --- a/math/atest-exp2.c +++ b/math/atest-exp2.c @@ -92,7 +92,7 @@ exp_mpn (mp1 ex, mp1 x) unsigned int n; mp1 xp; mp2 tmp; - mp_limb_t chk; + mp_limb_t chk __attribute__((__unused__)); mp1 tol; memset (xp, 0, sizeof (mp1));