From owner-freebsd-hackers@FreeBSD.ORG Wed Sep 19 09:19:23 2012 Return-Path: Delivered-To: hackers@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 9D077106566B for ; Wed, 19 Sep 2012 09:19:23 +0000 (UTC) (envelope-from erik@cederstrand.dk) Received: from csmtp2.one.com (csmtp2.one.com [91.198.169.22]) by mx1.freebsd.org (Postfix) with ESMTP id 606668FC15 for ; Wed, 19 Sep 2012 09:19:23 +0000 (UTC) Received: from [192.168.1.18] (unknown [217.157.7.221]) by csmtp2.one.com (Postfix) with ESMTPA id 2DCF23078C32 for ; Wed, 19 Sep 2012 09:19:16 +0000 (UTC) From: Erik Cederstrand Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Message-Id: <63FCC95F-7D73-423D-892D-DB5D75BCEBE7@cederstrand.dk> Date: Wed, 19 Sep 2012 11:19:15 +0200 To: FreeBSD Hackers Mime-Version: 1.0 (Mac OS X Mail 6.0 \(1486\)) X-Mailer: Apple Mail (2.1486) Cc: Subject: NDEBUG and assert() X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Sep 2012 09:19:23 -0000 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-amd6= 4/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+=3D -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=3D0. 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- lib/ncurses/config.mk (revision 240638) +++ lib/ncurses/config.mk (working copy) @@ -27,8 +27,6 @@ =20 CFLAGS+=3D -Wall =20 -CFLAGS+=3D -DNDEBUG - CFLAGS+=3D -DHAVE_CONFIG_H =20 # everyone needs this Index: include/assert.h =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- include/assert.h (revision 240638) +++ include/assert.h (working copy) @@ -45,7 +45,7 @@ #undef assert #undef _assert =20 -#ifdef NDEBUG +#if NDEBUG #define assert(e) ((void)0) #define _assert(e) ((void)0) #else