fix -Werror regressions with gcc 4.8 on x86_64

  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(-)
  

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.