diff mbox

Fix assert() warning in gcc < 4.8 [BZ# 21242]

Message ID 6619e7e2-248e-08bc-6dc5-7b4d11bea3df@auburn.edu
State New, archived
Headers

Commit Message

Justin Brewer March 19, 2017, 9:44 p.m. UTC 
  When compiling with gcc-4.7 and earlier, with -pedantic but without -ansi, a
warning is produced due to the braced group in the assert macro. This 
warning
also occurs in the latest clang. In gcc >= 4.8, no warning is produced.
Therefore, further restrict the usage of the braced group version of the 
macro
to gcc 4.8 and later. This patch also fixes the warning in clang, since
clang-3.9 sets the __GNUC_* version macros to mimic gcc 4.2.1

main.c:
  #include <assert.h>
  int main() { assert(1); }

Without patched assert.h:

$ gcc-4.8 -pedantic main.c
$ gcc-4.7 -pedantic main.c
main.c: In function ‘main’:
main.c:2:14: warning: ISO C forbids braced-groups within expressions 
[-pedantic]
$ gcc-4.4 -pedantic main.c
main.c: In function ‘main’:
main.c:2: warning: ISO C forbids braced-groups within expressions
$ clang-3.9 -pedantic main.c
main.c:2:14: warning: use of GNU statement expression extension 
[-Wgnu-statement-expression]
int main() { assert(1); }
              ^
/usr/include/assert.h:95:6: note: expanded from macro 'assert'
({ \
      ^
1 warning generated.

With patched assert.h:

$ gcc-4.8 -pedantic main.c
$ gcc-4.7 -pedantic main.c
$ gcc-4.4 -pedantic main.c
$ clang-3.9 -pedantic main.c
---
  assert/assert.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Florian Weimer March 21, 2017, 9:55 a.m. UTC | #1 
On 03/19/2017 10:44 PM, Justin Brewer wrote:
> -# if !defined __GNUC__ || defined __STRICT_ANSI__
> +# if !(defined __GNUC__ && __GNUC_PREREQ(4,8)) || defined __STRICT_ANSI__

I'm not happy how this disables the useful = warning for clang altogether.

Do we have a better option?  Is there a way for check for the -pedantic 
flag?

Thanks,
Florian
Joseph Myers March 21, 2017, 4:21 p.m. UTC | #2 
On Tue, 21 Mar 2017, Florian Weimer wrote:

> On 03/19/2017 10:44 PM, Justin Brewer wrote:
> > -# if !defined __GNUC__ || defined __STRICT_ANSI__
> > +# if !(defined __GNUC__ && __GNUC_PREREQ(4,8)) || defined __STRICT_ANSI__
> 
> I'm not happy how this disables the useful = warning for clang altogether.
> 
> Do we have a better option?  Is there a way for check for the -pedantic flag?

By design, warning options such as -pedantic are not meant to affect code 
generation, so should not have any way to check for them within the code.

(Preferably __extension__ should be used, but that runs into the issues 
with disabling it for the user's expression as discussed earlier for this 
code <https://sourceware.org/ml/libc-alpha/2016-11/msg00895.html>.)
Florian Weimer June 25, 2017, 4:02 p.m. UTC | #3 
On 03/21/2017 05:21 PM, Joseph Myers wrote:
> On Tue, 21 Mar 2017, Florian Weimer wrote:
> 
>> On 03/19/2017 10:44 PM, Justin Brewer wrote:
>>> -# if !defined __GNUC__ || defined __STRICT_ANSI__
>>> +# if !(defined __GNUC__ && __GNUC_PREREQ(4,8)) || defined __STRICT_ANSI__
>>
>> I'm not happy how this disables the useful = warning for clang altogether.
>>
>> Do we have a better option?  Is there a way for check for the -pedantic flag?
> 
> By design, warning options such as -pedantic are not meant to affect code 
> generation, so should not have any way to check for them within the code.
> 
> (Preferably __extension__ should be used, but that runs into the issues 
> with disabling it for the user's expression as discussed earlier for this 
> code <https://sourceware.org/ml/libc-alpha/2016-11/msg00895.html>.)

I think we can use __extension__ if we also expand the expression
without __extension__ in an unevaluated context.  The tricky part is to
find one that is independent of GNU extensions.
Perhaps this would work?

#  define assert(expr)                                           \
  ((void) sizeof ((expr) == 0), __extension__ ({                 \
      if (expr)                                                  \
        ; /* empty */                                            \
      else                                                       \
        __assert_fail (#expr, __FILE__, __LINE__, __FUNCTION__); \
    }))

sizeof suppresses the evaluation of the first occurrence of expr.  The
comparison is needed because sizeof cannot be applied to function
pointers and bitfields.  C11 says that expr is compared to zero, so the
(expr) == 0 expression is well-formed.

What do you think?  Should we make this change?

Thanks,
Florian
Joseph Myers June 26, 2017, 10:56 a.m. UTC | #4 
On Sun, 25 Jun 2017, Florian Weimer wrote:

> I think we can use __extension__ if we also expand the expression
> without __extension__ in an unevaluated context.  The tricky part is to
> find one that is independent of GNU extensions.
> Perhaps this would work?
> 
> #  define assert(expr)                                           \
>   ((void) sizeof ((expr) == 0), __extension__ ({                 \
>       if (expr)                                                  \
>         ; /* empty */                                            \
>       else                                                       \
>         __assert_fail (#expr, __FILE__, __LINE__, __FUNCTION__); \
>     }))
> 
> sizeof suppresses the evaluation of the first occurrence of expr.  The
> comparison is needed because sizeof cannot be applied to function
> pointers and bitfields.  C11 says that expr is compared to zero, so the
> (expr) == 0 expression is well-formed.
> 
> What do you think?  Should we make this change?

I think that's reasonable (appropriately commented to explain why it's 
done that way).
Florian Weimer July 4, 2017, 2 p.m. UTC | #5 
On 06/26/2017 12:56 PM, Joseph Myers wrote:
> On Sun, 25 Jun 2017, Florian Weimer wrote:
> 
>> I think we can use __extension__ if we also expand the expression
>> without __extension__ in an unevaluated context.  The tricky part is to
>> find one that is independent of GNU extensions.
>> Perhaps this would work?
>>
>> #  define assert(expr)                                           \
>>   ((void) sizeof ((expr) == 0), __extension__ ({                 \
>>       if (expr)                                                  \
>>         ; /* empty */                                            \
>>       else                                                       \
>>         __assert_fail (#expr, __FILE__, __LINE__, __FUNCTION__); \
>>     }))
>>
>> sizeof suppresses the evaluation of the first occurrence of expr.  The
>> comparison is needed because sizeof cannot be applied to function
>> pointers and bitfields.  C11 says that expr is compared to zero, so the
>> (expr) == 0 expression is well-formed.
>>
>> What do you think?  Should we make this change?
> 
> I think that's reasonable (appropriately commented to explain why it's 
> done that way).

This is what I came up with as a patch.

I would consider this still appropriate during the freeze.  It's
something we'd backport to glibc 2.25, too.

Thanks,
Florian
diff mbox

Patch

diff --git a/assert/assert.h b/assert/assert.h
index 22f019537c..88dde76414 100644
--- a/assert/assert.h
+++ b/assert/assert.h
@@ -85,7 +85,7 @@  __END_DECLS
  /* 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__
+# if !(defined __GNUC__ && __GNUC_PREREQ(4,8)) || defined __STRICT_ANSI__
  #  define assert(expr)                            \
      ((expr)                                \
       ? __ASSERT_VOID_CAST (0)                        \