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

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

[Jim Meyering]

From: Jim Meyering <meyering@fb.com>

* 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 the parentheses added by assert.
The new definition uses "if (expr) ; else __assert_fail...", so
gcc -Wall will now detect that type of error in an assert, too.
---
 assert/assert.h | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/assert/assert.h b/assert/assert.h
index 2661391..ffcf60c 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 (__STRING(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 (__STRING(expr), __FILE__, __LINE__, __ASSERT_FUNCTION))
+# else
+#  define assert(expr)							\
+    ({									\
+      if (expr)								\
+        ; /* empty */							\
+      else								\
+        __assert_fail (__STRING(expr), __FILE__, __LINE__, __ASSERT_FUNCTION);\
+    })
+# endif

 # ifdef	__USE_GNU
 #  define assert_perror(errnum)						\
-- 
2.0.1

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Roland McGrath]

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

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.

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.

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


Thanks,
Roland

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Jim Meyering]

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.

Thanks,
Jim

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Jim Meyering]

[-- Attachment #1: Type: text/plain, Size: 3534 bytes --]

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?

[-- Attachment #2: glibc-assert--do-not-suppress-warning-about-accidental-assignment.diff --]
[-- Type: text/plain, Size: 2072 bytes --]

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

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

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Zack Weinberg]

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

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Joseph Myers]

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__.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Jim Meyering]

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

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Florian Weimer]

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

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Jim Meyering]

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

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Jim Meyering]

[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]

On Fri, Nov 25, 2016 at 10:14 PM, Jim Meyering <jim@meyering.net> wrote:
> 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

Here's the complete, rebased patch. Ok to push, presuming I still have
commit access?

[-- Attachment #2: glibc-assert--do-not-suppress-warning-about-accidental-assignment.diff --]
[-- Type: text/plain, Size: 3132 bytes --]

From 0954feae6411cc0de5f5cb6c7e007b972388139f 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.
---
 ChangeLog       | 15 +++++++++++++++
 assert/assert.h | 21 +++++++++++++++++----
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e19db5d..a0181e6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+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
+
 2016-12-08  Joseph Myers  <joseph@codesourcery.com>

 	* Rules [$(run-built-tests) != no] (tests-expected): Add
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)						\
-- 
2.9.3

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Jim Meyering]

On Thu, Dec 8, 2016 at 7:17 PM, Jim Meyering <jim@meyering.net> wrote:
> On Fri, Nov 25, 2016 at 10:14 PM, Jim Meyering <jim@meyering.net> wrote:
>> 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
>
> Here's the complete, rebased patch. Ok to push, presuming I still have
> commit access?

Friendly pre-holiday ping?

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Florian Weimer]

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

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Jim Meyering]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

On Wed, Dec 14, 2016 at 1:13 AM, Florian Weimer <fweimer@redhat.com> wrote:
> 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?

Thanks for the review.

Good point. It's really for -ansi -pedantic, I suppose. Updated the log message.

>>         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. I didn't realize glibc used such a strict interpretation.
Happy to adjust. How's this (also in the attached, rebased commit):

        Let gcc detect assert(a = 1) errors.
        * assert/assert.h (assert) Rewrite, retaining the old definintion
        when required, but otherwise putting the expression as-is in an "if"
        expression (hence, with no added parentheses) within a statement
        expression.

[-- Attachment #2: glibc-assert--do-not-suppress-warning-about-accidental-assignment.diff --]
[-- Type: text/plain, Size: 2604 bytes --]

From 9cb1a7fd2ac0d7afc62e5a1e742c3819e1ac4346 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 make this work also with both
-ansi and  -pedantic, which would reject 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.
---
 ChangeLog       |  8 ++++++++
 assert/assert.h | 21 +++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c08b711..0779419 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2016-11-25  Jim Meyering  <meyering@fb.com>
+
+	Let gcc detect assert(a = 1) errors.
+	* assert/assert.h (assert) Rewrite, retaining the old definintion
+	when required, but otherwise putting the expression as-is in an "if"
+	expression (hence, with no added parentheses) within a statement
+	expression.
+
 2016-12-14  Joseph Myers  <joseph@codesourcery.com>

 	[BZ #20947]
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)						\
-- 
2.9.3

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Florian Weimer]

On 12/15/2016 01:24 AM, Jim Meyering wrote:

> Thanks. I didn't realize glibc used such a strict interpretation.
> Happy to adjust. How's this (also in the attached, rebased commit):
>
>         Let gcc detect assert(a = 1) errors.
>         * assert/assert.h (assert) Rewrite, retaining the old definintion
>         when required, but otherwise putting the expression as-is in an "if"
>         expression (hence, with no added parentheses) within a statement
>         expression.

Thanks.  I just noticed your fb.com address in the ChangeLog.  Do you 
know if Facebook have filed paperwork with the FSF for glibc?  Commit 
7ee03f00188723a4de2b85021e511ced6d7fc4be was just two lines, and this 
commit here only adds only very few significant source lines, too, so we 
might get away without the paperwork, but this is probably the last such 
change we can accept.

Anyway, please install if you have commit access.

Thanks,
Florian

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Jim Meyering]

On Fri, Dec 16, 2016 at 4:15 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 12/15/2016 01:24 AM, Jim Meyering wrote:
>
>> Thanks. I didn't realize glibc used such a strict interpretation.
>> Happy to adjust. How's this (also in the attached, rebased commit):
>>
>>         Let gcc detect assert(a = 1) errors.
>>         * assert/assert.h (assert) Rewrite, retaining the old definintion
>>         when required, but otherwise putting the expression as-is in an
>> "if"
>>         expression (hence, with no added parentheses) within a statement
>>         expression.
>
>
> Thanks.  I just noticed your fb.com address in the ChangeLog.  Do you know
> if Facebook have filed paperwork with the FSF for glibc?  Commit
> 7ee03f00188723a4de2b85021e511ced6d7fc4be was just two lines, and this commit
> here only adds only very few significant source lines, too, so we might get
> away without the paperwork, but this is probably the last such change we can
> accept.
>
> Anyway, please install if you have commit access.

Thanks. Pushed.
I'll add glibc to my list for the next Facebook copyright assignment.

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Andreas Schwab]

On Dez 14 2016, Jim Meyering <jim@meyering.net> wrote:

> Thanks. I didn't realize glibc used such a strict interpretation.
> Happy to adjust. How's this (also in the attached, rebased commit):
>
>         Let gcc detect assert(a = 1) errors.
>         * assert/assert.h (assert) Rewrite, retaining the old definintion

Missing colon.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] assert.h: allow gcc to detect assert(a = 1) errors

[Florian Weimer]

On 12/27/2016 06:47 PM, Andreas Schwab wrote:
> On Dez 14 2016, Jim Meyering <jim@meyering.net> wrote:
>
>> Thanks. I didn't realize glibc used such a strict interpretation.
>> Happy to adjust. How's this (also in the attached, rebased commit):
>>
>>         Let gcc detect assert(a = 1) errors.
>>         * assert/assert.h (assert) Rewrite, retaining the old definintion
>
> Missing colon.

I fixed this along with a couple of other ChangeLog typos.

Thanks,
Florian