diff mbox

assert.h: allow gcc to detect assert(a = 1) errors

Message ID CA+8g5KFy4tk+H3t0BKoe=wQqsW+ea3ZtzOe2bb+xBUNbtxGBWg@mail.gmail.com
State New
Headers show

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

Zack Weinberg Nov. 24, 2016, 2:59 a.m. UTC | #1
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
Florian Weimer Nov. 24, 2016, 7:36 a.m. UTC | #2
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
Joseph Myers Nov. 24, 2016, 1:12 p.m. UTC | #3
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__.)
Jim Meyering Nov. 26, 2016, 6:14 a.m. UTC | #4
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
Jim Meyering Nov. 26, 2016, 6:35 a.m. UTC | #5
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
Florian Weimer Dec. 14, 2016, 9:13 a.m. UTC | #6
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 mbox

Patch

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)						\