diff mbox

[B1/2] Redefine assert to use its expression

Message ID 1420827419-18655-2-git-send-email-rth@twiddle.net
State New, archived
Headers show

Commit Message

Richard Henderson Jan. 9, 2015, 6:16 p.m. UTC
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(-)

Comments

Mike Frysinger March 2, 2015, 7:44 a.m. UTC | #1
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
Roland McGrath March 2, 2015, 10:52 p.m. UTC | #2
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
Rich Felker March 2, 2015, 11:18 p.m. UTC | #3
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
Roland McGrath March 2, 2015, 11:23 p.m. UTC | #4
> 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.
Rich Felker March 2, 2015, 11:33 p.m. UTC | #5
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
Roland McGrath March 2, 2015, 11:45 p.m. UTC | #6
> 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.
diff mbox

Patch

diff --git a/elf/rtld.c b/elf/rtld.c
index 69873c2..0772a78 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -1391,9 +1391,7 @@  ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
 		  /* We cannot use the DSO, it does not have the
 		     appropriate interfaces or it expects something
 		     more recent.  */
-#ifndef NDEBUG
 		  Lmid_t ns = dlmargs.map->l_ns;
-#endif
 		  _dl_close (dlmargs.map);
 
 		  /* Make sure the namespace has been cleared entirely.  */
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
diff --git a/locale/programs/ld-collate.c b/locale/programs/ld-collate.c
index dc0fe30..ba620c9 100644
--- a/locale/programs/ld-collate.c
+++ b/locale/programs/ld-collate.c
@@ -2432,9 +2432,7 @@  collate_output (struct localedef_t *locale, const struct charmap_t *charmap,
 	  uint32_t namelen = strlen (runp->name);
 	  uint32_t hash = elem_hash (runp->name, namelen);
 	  size_t idx = hash % elem_size;
-#ifndef NDEBUG
 	  size_t start_idx = idx;
-#endif
 
 	  if (elem_table[idx * 2] != 0)
 	    {
diff --git a/sysdeps/nptl/fork.c b/sysdeps/nptl/fork.c
index 5cffd82..cf71fe1 100644
--- a/sysdeps/nptl/fork.c
+++ b/sysdeps/nptl/fork.c
@@ -110,9 +110,7 @@  __libc_fork (void)
 
   _IO_list_lock ();
 
-#ifndef NDEBUG
   pid_t ppid = THREAD_GETMEM (THREAD_SELF, tid);
-#endif
 
   /* We need to prevent the getpid() code to update the PID field so
      that, if a signal arrives in the child very early and the signal