Message ID | CA+8g5KFy4tk+H3t0BKoe=wQqsW+ea3ZtzOe2bb+xBUNbtxGBWg@mail.gmail.com |
---|---|
State | New, archived |
Headers |
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: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> 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: <CA+8g5KH_vG9KY-fT8miGH9oSULCoffd5DQQbgr-GDR6d2qTktA@mail.gmail.com> References: <1405537923-28692-1-git-send-email-jim@meyering.net> <20140716201505.34FF22C398F@topped-with-meat.com> <CA+8g5KH_vG9KY-fT8miGH9oSULCoffd5DQQbgr-GDR6d2qTktA@mail.gmail.com> From: Jim Meyering <jim@meyering.net> Date: Wed, 23 Nov 2016 18:21:58 -0800 Message-ID: <CA+8g5KFy4tk+H3t0BKoe=wQqsW+ea3ZtzOe2bb+xBUNbtxGBWg@mail.gmail.com> Subject: Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors To: Roland McGrath <roland@hack.frob.com> Cc: libc-alpha <libc-alpha@sourceware.org> Content-Type: multipart/mixed; boundary=001a1149bab4bff5e2054202acbc |
Commit Message
Jim Meyering
Nov. 24, 2016, 2:21 a.m. UTC
On Wed, Jul 16, 2014 at 1:43 PM, Jim Meyering <jim@meyering.net> wrote: > On Wed, Jul 16, 2014 at 1:15 PM, Roland McGrath <roland@hack.frob.com> 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 <assert.h>' \ '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 <assert.h>' \ '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 <assert.h>' \ '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 <meyering@fb.com> 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(-)
Comments
On Wed, Nov 23, 2016 at 9:21 PM, Jim Meyering <jim@meyering.net> wrote: [...] Not commenting on anything else, but... > 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; } ... Would you please file a feature-request with the gcc people asking for something that turns __extension__ back off? It would be nice to be able to get this right even in __STRICT_ANSI__ mode. I'm imagining that your macro could look like #define assert(expr) \ __extension__ ({ \ if (__noextension__ (expr)) \ ; \ else __assert_failed (...); \ (void)0; \ }) (Make clear that the parentheses in __noextension__(...) need to not count for the purpose of -Wparentheses.) zw
On 11/24/2016 03:21 AM, Jim Meyering wrote: > 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; } Agreed. > Tests I ran manually in a directory with the new assert.h file: > Do you require a test suite addition for these? If so, would a single > bourne shell script be acceptable? We currently lack the machinery for that. It's not just that it would need a shell script. We also do not compile tests with headers as system headers. The patch looks good to me, but it needs a ChangeLog entry. Thanks, Florian
On Wed, 23 Nov 2016, Zack Weinberg wrote: > ... Would you please file a feature-request with the gcc people asking > for something that turns __extension__ back off? It would be nice to (Which I suggested in <https://gcc.gnu.org/ml/gcc/2001-04/msg00642.html> - note the point about counting nesting so this cancels exactly one enclosing __extension__.)
On Wed, Nov 23, 2016 at 11:36 PM, Florian Weimer <fweimer@redhat.com> wrote: > On 11/24/2016 03:21 AM, Jim Meyering wrote: > >> 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; } > > > Agreed. > >> Tests I ran manually in a directory with the new assert.h file: > > >> Do you require a test suite addition for these? If so, would a single >> bourne shell script be acceptable? > > > We currently lack the machinery for that. It's not just that it would need > a shell script. We also do not compile tests with headers as system > headers. > > The patch looks good to me, but it needs a ChangeLog entry. Thanks for the review. Here's a proposed ChangeLog entry: 2016-11-25 Jim Meyering <meyering@fb.com> Let gcc 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. https://bugzilla.redhat.com/1105335
On Thu, Nov 24, 2016 at 5:12 AM, Joseph Myers <joseph@codesourcery.com> wrote: > On Wed, 23 Nov 2016, Zack Weinberg wrote: > >> ... Would you please file a feature-request with the gcc people asking >> for something that turns __extension__ back off? It would be nice to > > (Which I suggested in <https://gcc.gnu.org/ml/gcc/2001-04/msg00642.html> - > note the point about counting nesting so this cancels exactly one > enclosing __extension__.) I made the feature request here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78539
On 11/26/2016 07:14 AM, Jim Meyering wrote: > Here's a proposed ChangeLog entry: > > 2016-11-25 Jim Meyering <meyering@fb.com> > > Let gcc 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 ({...}). Isn't this warning suppressed for system headers with -Wpedantic? > 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. > https://bugzilla.redhat.com/1105335 The following is a bit silly. glibc adheres to the GNU standards, which require that the ChangeLog only describes the change as such, not its rationale. So the entry should probably look like this. Let GCC detect assert (a = 1) errors. * assert/assert.h (assert): Introduce __STRICT_ANSI__ variant. Use a GCC extension and an if statement for GNU compilers. Thanks, Florian
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) \