NDEBUG and assert()

[Erik Cederstrand]

Hello,

I'm sifting through the Clang Analyzer reports and decided to take a look at this one: http://scan.freebsd.your.org/freebsd-head/lib.ncurses.menu/2012-09-16-amd64/report-3vc5Zu.html#Path5

If you mouseover the assert() in line 236, the problem is that assert() resolves to "(void)0" so the analyzer doesn't know that item is non-null later on. Since this is contrib code, my best bet to fix it is to enable assert()'s in our build code for ncurses. That way, users will get an assertion error describing the real problem, instead of a null pointer dereference or garbage data. My assumption here is that nurses is not performance-sensitive so an extra assert() here and there is OK.

lib/ncurses/config.mk has "CFLAGS+= -DNDEBUG". If I remove this, contrib/ncurses/include/MKncurses_def.sh just makes sure NDEBUG is set to 0 instead of 1. However, include/assert.h just tests for "ifdef NDEBUG" to decide whether to actually assert or just return (void)0, so it also returns (void)0 when NDEBUG=0. This was obviously not what the ncurses author intended. By changing "ifdef NDEBUG" to "if NDEBUG" the value of NDEBUG is evaluated to true or false.

The below below patch will let the analyzer reason correctly about the code, and removes the report mentioned above (and a handful others in ncurses). It doesn't touch contrib code, but I'm not happy about changing include/assert.h since it's used so many other places. Any other ideas for how to best solve this?

Kind regards,
Erik Cederstrand


Index: lib/ncurses/ncurses/ncurses_cfg.h
===================================================================
--- lib/ncurses/ncurses/ncurses_cfg.h (revision 240638)
+++ lib/ncurses/ncurses/ncurses_cfg.h (working copy)
@@ -145,7 +145,7 @@
#define NCURSES_NO_PADDING 1
#define NCURSES_PATHSEP ':'
#define NCURSES_VERSION_STRING "5.7.20081102"
-#define NDEBUG 1
+#define NDEBUG 0
#define RETSIGTYPE void
#define SIG_ATOMIC_T volatile sig_atomic_t
#define SIZEOF_SIGNED_CHAR 1
Index: lib/ncurses/config.mk
===================================================================
--- lib/ncurses/config.mk (revision 240638)
+++ lib/ncurses/config.mk (working copy)
@@ -27,8 +27,6 @@

CFLAGS+= -Wall

-CFLAGS+= -DNDEBUG
-
CFLAGS+= -DHAVE_CONFIG_H

# everyone needs this
Index: include/assert.h
===================================================================
--- include/assert.h (revision 240638)
+++ include/assert.h (working copy)
@@ -45,7 +45,7 @@
#undef assert
#undef _assert

-#ifdef NDEBUG
+#if NDEBUG
#define assert(e) ((void)0)
#define _assert(e) ((void)0)
#else

_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hacke...@freebsd.org"

[Erik Cederstrand]

Den 19/09/2012 kl. 11.19 skrev Erik Cederstrand <er...@cederstrand.dk>:

> The below below patch will let the analyzer reason correctly about the code, and removes the report mentioned above (and a handful others in ncurses). It doesn't touch contrib code, but I'm not happy about changing include/assert.h since it's used so many other places. Any other ideas for how to best solve this?

An alternative that doesn't touch assert.h but contains a patch to /contrib:

Erik

Index: lib/ncurses/ncurses/ncurses_cfg.h
===================================================================
--- lib/ncurses/ncurses/ncurses_cfg.h (revision 240638)
+++ lib/ncurses/ncurses/ncurses_cfg.h (working copy)

@@ -145,7 +145,6 @@

#define NCURSES_NO_PADDING 1
#define NCURSES_PATHSEP ':'
#define NCURSES_VERSION_STRING "5.7.20081102"
-#define NDEBUG 1

#define RETSIGTYPE void
#define SIG_ATOMIC_T volatile sig_atomic_t
#define SIZEOF_SIGNED_CHAR 1
Index: lib/ncurses/config.mk
===================================================================
--- lib/ncurses/config.mk (revision 240638)
+++ lib/ncurses/config.mk (working copy)
@@ -27,8 +27,6 @@

CFLAGS+= -Wall

-CFLAGS+= -DNDEBUG
-
CFLAGS+= -DHAVE_CONFIG_H

# everyone needs this

Index: contrib/ncurses/include/ncurses_defs
===================================================================
--- contrib/ncurses/include/ncurses_defs (revision 240638)
+++ contrib/ncurses/include/ncurses_defs (working copy)
@@ -171,7 +171,6 @@
NCURSES_EXT_FUNCS
NCURSES_NO_PADDING
NCURSES_PATHSEP ':'
-NDEBUG
NEED_PTEM_H
NO_LEAKS
PURE_TERMINFO

[Garrett Cooper]

On Wed, Sep 19, 2012 at 3:18 AM, Erik Cederstrand <er...@cederstrand.dk> wrote:
> Den 19/09/2012 kl. 11.19 skrev Erik Cederstrand <er...@cederstrand.dk>:
>
>> The below below patch will let the analyzer reason correctly about the code, and removes the report mentioned above (and a handful others in ncurses). It doesn't touch contrib code, but I'm not happy about changing include/assert.h since it's used so many other places. Any other ideas for how to best solve this?
>
> An alternative that doesn't touch assert.h but contains a patch to /contrib:

This should be disabled [in part because it can be properly
enabled with WITH_ASSERT_DEBUG=yes in /etc/src.conf].
Thanks!
-Garrett

[dic...@his.com]

That matches upstream :-)