[B1/2] Redefine assert to use its expression

  This also fixes the -Werrors from -DNDEBUG on x86_64, but in
the opposite direction.  We make assert always use its expression.
This requires removing some existing ifndefs that were working
around this same problem of unused variables.

There is exactly one compilation difference from patch A1, in
fork.os, where the THREAD_GETMEM read cannot be optimized away,
because the asm is marked volatile.  (Yet another reason to finish
code in gcc to expose segments as address spaces, so that this can
be written in C.)


r~


	* include/assert.h (assert): Redefine to use the argument.
	(assert_perror): Likewise.
	* elf/rtld.c (dl_main): Don't check NDEBUG.
	* locale/programs/ld-collate.c (collate_output): Likewise.
	* sysdeps/nptl/fork.c (__libc_fork): Likewise.

---
 elf/rtld.c                   | 2 --
 include/assert.h             | 7 +++++++
 locale/programs/ld-collate.c | 2 --
 sysdeps/nptl/fork.c          | 2 --
 4 files changed, 7 insertions(+), 6 deletions(-)
  

On 09 Jan 2015 10:16, Richard Henderson wrote:
> This also fixes the -Werrors from -DNDEBUG on x86_64, but in
> the opposite direction.  We make assert always use its expression.
> This requires removing some existing ifndefs that were working
> around this same problem of unused variables.

i like this variant as it encourages us to avoid ifdefs, and does so w/out 
impacting code generation in general
-mike
  

I thought I argued before for using a new internal macro instead of
overloading assert with different semantics.  That's more immediate
make-work of replacing existing assert uses, but IMHO more maintainable in
the long run.  But I don't recall any more exactly how the previous
discussion went.

> This also fixes the -Werrors from -DNDEBUG on x86_64, but in
> the opposite direction.  We make assert always use its expression.
> This requires removing some existing ifndefs that were working
> around this same problem of unused variables.

This is the approach I like, certainly, regardless of whether we decide to
change assert itself or use a new macro name.

> There is exactly one compilation difference from patch A1, in
> fork.os, where the THREAD_GETMEM read cannot be optimized away,
> because the asm is marked volatile.  (Yet another reason to finish
> code in gcc to expose segments as address spaces, so that this can
> be written in C.)

Not to discourage you from implementing something in GCC, but why do those
asm's use volatile?  I don't see a reason they should.  Obviously the
storing ones need to, but the fetching ones shouldn't.


Thanks,
Roland
  

On Fri, Jan 09, 2015 at 10:16:58AM -0800, Richard Henderson wrote:
> This also fixes the -Werrors from -DNDEBUG on x86_64, but in
> the opposite direction.  We make assert always use its expression.
> This requires removing some existing ifndefs that were working
> around this same problem of unused variables.
> 
> There is exactly one compilation difference from patch A1, in
> fork.os, where the THREAD_GETMEM read cannot be optimized away,
> because the asm is marked volatile.  (Yet another reason to finish
> code in gcc to expose segments as address spaces, so that this can
> be written in C.)
> [...]
> diff --git a/include/assert.h b/include/assert.h
> index c452667..d7e2759 100644
> --- a/include/assert.h
> +++ b/include/assert.h
> @@ -25,3 +25,10 @@ hidden_proto (__assert_fail)
>  hidden_proto (__assert_perror_fail)
>  # endif
>  #endif
> +
> +#ifdef NDEBUG
> +# undef assert
> +# undef assert_perror
> +# define assert(expr)		((void)(0 && (expr)))
> +# define assert_perror(errnum)	((void)(0 && (errnum)))
> +#endif

This change is non-conforming to the requirements of ISO C, which
state explicitly (C11 7.2p1):

    If NDEBUG is defined as a macro name at the point in the source
    file where <assert.h> is included, the assert macro is defined
    simply as

        #define assert(ignore) ((void)0)

There is no allowance for alternate definitions. In particular, the
following program is valid C:

#define NDEBUG
int main() { assert(+++++++++++++++); }

There are presumably also ways you can observe a wrong definition with
stringifying operations at the preprocessor level.

Rich
  

> This change is non-conforming to the requirements of ISO C, which
> state explicitly (C11 7.2p1):

You missed the part where this is only for libc-internal uses and
has nothing to do with the installed <assert.h>.  Such confusion
is why I'm inclined to use a new macro name instead.
  

On Mon, Mar 02, 2015 at 03:23:21PM -0800, Roland McGrath wrote:
> > This change is non-conforming to the requirements of ISO C, which
> > state explicitly (C11 7.2p1):
> 
> You missed the part where this is only for libc-internal uses and
> has nothing to do with the installed <assert.h>.  Such confusion
> is why I'm inclined to use a new macro name instead.

Yes, I did. Sorry for the noise then. Actually I thought about that
after sending, but then I couldn't figure out from the context whether
it was internal or not so I figured I'd wait and see if anyone else
had something to say.

Rich
  

> Yes, I did. Sorry for the noise then. Actually I thought about that
> after sending, but then I couldn't figure out from the context whether
> it was internal or not so I figured I'd wait and see if anyone else
> had something to say.

Most files in include/ are internal wrapper headers.