From patchwork Thu Nov 24 02:21:58 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Jim Meyering X-Patchwork-Id: 17821 Received: (qmail 51264 invoked by alias); 24 Nov 2016 02:22:32 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 51130 invoked by uid 89); 24 Nov 2016 02:22:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=kc, HTo:U*roland, Roland, vacation X-HELO: mail-io0-f193.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=wsolc+51JCjWTkzl6tO3WXaMH4+DlQIZSoPCPkSgWxU=; b=b/Ozvqh5OFOKKDJalIQbp47AFxSJ4jcviYVwolTwRKznbuATw2joKogNFR+u5HIilI DeBvwnCpQi3pqV8yx40Ru/hK4hdioaAxzQQUaJt/7KmGY/EXt1AlERVofiwb9BBBuf/x AqrXOzyU2T2UI1AD1RRI+4AcVTx0xTH7G378VPEpyGh/961T1R8Qq8xqhDJneTtqJ/jy A5ZxPYN55dkrp2wg3uIScIYY9KE+kJG9+NrEscgil/lZ+UlnPd+A9EmXeAdIjrDnhRsw U114OhP5QK60mdZjPV/X09jI59ZH1uW1ksSIg6SXmKgA4XiKqT3AkKz54zAO74NTvPst LaIg== X-Gm-Message-State: AKaTC02Qg9Ly/e0CROc36rBQ6ZaLJ9weDFlhrM5XqPiBzflJkJVv7Tzm3t+4HRunYPVO+vhe0Kfz1RbE4K4oow== X-Received: by 10.36.214.67 with SMTP id o64mr10535itg.31.1479954139173; Wed, 23 Nov 2016 18:22:19 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1405537923-28692-1-git-send-email-jim@meyering.net> <20140716201505.34FF22C398F@topped-with-meat.com> From: Jim Meyering Date: Wed, 23 Nov 2016 18:21:58 -0800 Message-ID: Subject: Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors To: Roland McGrath Cc: libc-alpha On Wed, Jul 16, 2014 at 1:43 PM, Jim Meyering wrote: > On Wed, Jul 16, 2014 at 1:15 PM, Roland McGrath wrote: >> This certainly should not go in during the current release freeze. >> So you might not want to try to get too much discussion going until >> the floodgates reopen. Also, various key people might be less than >> usually responsive until after Cauldron (and for me, another week of >> vacation thereafter). > > Thanks for the quick feedback. > >> Isn't there an "empty statement" warning that might be on too? We >> certainly don't want the internals of the macro (the then clause in >> your new version) to produce warnings of their own with any possible >> combination of switches. > > -Wempty-body does not complain about the empty "then" clause > inside that statement-expression. > >> You should be able to use __extension__ for GCC under -ansi. But >> perhaps that would wrongly allow use of extensions inside the user's >> expression too. If you're avoiding it for good reason, there should >> be comments there explaining why. > > Good point. I confirmed that prefixing the ({... with __extension__ > lets me eliminate the __STRICT_ANSI__ disjunct, and that -ansi > still warns about a use like "assert( ({1}) );". I'll update the patch > and repost here -- with a detailed test description -- > in a couple weeks. > >> I also wonder how modern GCCs running in integrated preprocessor >> mode deal with the system header exception and/or __extension__ for >> this sort of case (since in integrated preprocessor mode it can tell >> which part came from the system header and which from the user). >> >> Any change like this is going to need a lot of detailed reporting >> about testing with different compilers and option combinations and >> sorts of expressions (i.e. ones using extensions or not and perhaps >> various different sorts of extensions). > > I've tested it with -Wextra -Wall -W -O, with uses as statements and > as expressions. Also with and without -ansi. I have not yet tested > with a non-__GNUC__ compiler, but will do. Hi Roland, This slipped off my radar (for two years!). I've attached an updated patch. We *do* need that __STRICT_ANSI__ disjunct. Otherwise, this would evoke no warning: $ gcc -isystem. -I. -Werror=pedantic k.c In file included from k.c:1:0: k.c: In function ‘main’: k.c:2:23: warning: ISO C forbids braced-groups within expressions [-Wpedantic] int main() { assert ( ({1;}) ); return 0; } Tests I ran manually in a directory with the new assert.h file: Ensure that "assert(a=0)" now elicits a warning and this fails: printf '%s\n' '#include ' \ 'int main() {int a = 1; assert (a = 0); return a;}' > k.c gcc -c -I. -Werror=parentheses k.c Ensure that clang does the same: clang -c -I. -Werror=parentheses k.c Ensure that this fails with a diagnostic about the stmt expression: printf '%s\n' '#include ' \ 'int main() { assert (({1;})); return 0; }' > k.c gcc -c -isystem. -I. -Werror=pedantic k.c Ensure -Werror=pedantic does not complain about the stmt expression: printf '%s\n' '#include ' \ 'int main() { assert (0); return 0; }' > k.c gcc -isystem. -I. -Werror=pedantic k.c I didn't see any other tests for failing compilation. Do you require a test suite addition for these? If so, would a single bourne shell script be acceptable? From 70387babebf4213baca2023adcf563df4aa7dbd0 Mon Sep 17 00:00:00 2001 From: Jim Meyering Date: Thu, 5 Jun 2014 10:42:05 -0700 Subject: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors * assert/assert.h (assert): Rewrite assert's definition so that a s/==/=/ typo, e.g., assert(errno = ENOENT) is not hidden from gcc's -Wparentheses by assert-added parentheses. The new definition uses "if (expr) /* empty */; else __assert_fail...", so gcc -Wall will now detect that type of error in an assert, too. The __STRICT_ANSI__ disjunct is to avoid the warning that -Wpedantic would otherwise issue for the use of ({...}). I would have preferred to use __extension__ to mark that, but doing so would mistakenly suppress warnings about any extension in the user-supplied "expr". E.g., "assert ( ({1;}) )" must continue to evoke a warning. --- assert/assert.h | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/assert/assert.h b/assert/assert.h index 729edeb..0f25131 100644 --- a/assert/assert.h +++ b/assert/assert.h @@ -82,10 +82,23 @@ extern void __assert (const char *__assertion, const char *__file, int __line) __END_DECLS -# define assert(expr) \ - ((expr) \ - ? __ASSERT_VOID_CAST (0) \ - : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION)) +/* When possible, define assert so that it does not add extra + parentheses around EXPR. Otherwise, those added parentheses would + suppress warnings we'd expect to be detected by gcc's -Wparentheses. */ +# if !defined __GNUC__ || defined __STRICT_ANSI__ +# define assert(expr) \ + ((expr) \ + ? __ASSERT_VOID_CAST (0) \ + : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION)) +# else +# define assert(expr) \ + ({ \ + if (expr) \ + ; /* empty */ \ + else \ + __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION); \ + }) +# endif # ifdef __USE_GNU # define assert_perror(errnum) \