Wrap a macro in do {} while (0) (PR sanitizer/80063)

Submitted by Marek Polacek on March 20, 2017, 1:28 p.m.

Details

Message ID 20170320132802.GH3172@redhat.com
State New
Headers show

Commit Message

Marek Polacek March 20, 2017, 1:28 p.m.
PVS-Studio tool complained about this and it's right, macros shouldn't expand
to multiple statements like this.

Bootstrapped/regtested on x86_64-linux, applying to trunk.

2017-03-20  Marek Polacek  <polacek@redhat.com>

	PR sanitizer/80063
	* asan.c (DEF_SANITIZER_BUILTIN): Use do { } while (0).


	Marek

Comments

Jason Merrill March 20, 2017, 1:37 p.m.
On Mon, Mar 20, 2017 at 9:28 AM, Marek Polacek <polacek@redhat.com> wrote:
>  #define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) \
> +  do {                                                                 \
> +    decl = add_builtin_function ("__builtin_" NAME, TYPE, ENUM,                \
> +                                BUILT_IN_NORMAL, NAME, NULL_TREE);     \
> +    set_call_expr_flags (decl, ATTRS);                                 \
> +    set_builtin_decl (ENUM, decl, true);                               \
> +  } while (0);

We leave out the trailing ; in a macro like this.

Jason
Jakub Jelinek March 20, 2017, 1:41 p.m.
On Mon, Mar 20, 2017 at 09:37:10AM -0400, Jason Merrill wrote:
> On Mon, Mar 20, 2017 at 9:28 AM, Marek Polacek <polacek@redhat.com> wrote:
> >  #define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) \
> > +  do {                                                                 \
> > +    decl = add_builtin_function ("__builtin_" NAME, TYPE, ENUM,                \
> > +                                BUILT_IN_NORMAL, NAME, NULL_TREE);     \
> > +    set_call_expr_flags (decl, ATTRS);                                 \
> > +    set_builtin_decl (ENUM, decl, true);                               \
> > +  } while (0);
> 
> We leave out the trailing ; in a macro like this.

If we only use them for code, sure.  But DEF_*BUILTIN is used also to
define the names, enum values etc. where it needs other separators, so
*.def files don't put any separator after the closing ).

	Jakub
Marek Polacek March 20, 2017, 1:42 p.m.
On Mon, Mar 20, 2017 at 09:37:10AM -0400, Jason Merrill wrote:
> On Mon, Mar 20, 2017 at 9:28 AM, Marek Polacek <polacek@redhat.com> wrote:
> >  #define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) \
> > +  do {                                                                 \
> > +    decl = add_builtin_function ("__builtin_" NAME, TYPE, ENUM,                \
> > +                                BUILT_IN_NORMAL, NAME, NULL_TREE);     \
> > +    set_call_expr_flags (decl, ATTRS);                                 \
> > +    set_builtin_decl (ENUM, decl, true);                               \
> > +  } while (0);
> 
> We leave out the trailing ; in a macro like this.

True, but that wasn't possible in this case, otherwise I got tons of

/home/marek/src/gcc/gcc/asan.c:2570:3: error: expected ‘;’ before ‘do’
   do {         \
   ^
/home/marek/src/gcc/gcc/sanitizer.def:459:1: note: in expansion of macro ‘DEF_SANITIZER_BUILTIN’
 DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH_ABORT

	Marek
Martin Sebor March 20, 2017, 4:44 p.m.
On 03/20/2017 07:42 AM, Marek Polacek wrote:
> On Mon, Mar 20, 2017 at 09:37:10AM -0400, Jason Merrill wrote:
>> On Mon, Mar 20, 2017 at 9:28 AM, Marek Polacek <polacek@redhat.com> wrote:
>>>  #define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) \
>>> +  do {                                                                 \
>>> +    decl = add_builtin_function ("__builtin_" NAME, TYPE, ENUM,                \
>>> +                                BUILT_IN_NORMAL, NAME, NULL_TREE);     \
>>> +    set_call_expr_flags (decl, ATTRS);                                 \
>>> +    set_builtin_decl (ENUM, decl, true);                               \
>>> +  } while (0);
>>
>> We leave out the trailing ; in a macro like this.
>
> True, but that wasn't possible in this case, otherwise I got tons of
>
> /home/marek/src/gcc/gcc/asan.c:2570:3: error: expected ‘;’ before ‘do’
>    do {         \
>    ^
> /home/marek/src/gcc/gcc/sanitizer.def:459:1: note: in expansion of macro ‘DEF_SANITIZER_BUILTIN’
>  DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH_ABORT

I would view these as helpful errors and expect them to be fixed
by terminating the macro invocations with a semicolon rather than
by adding it to the macro definition itself.  Is there a problem
with doing that that I'm not considering?

Martin

PS The GCC manual documents this problem in the section titled
Swallowing the Semicolon:
https://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html

This problem is also the subject of the CERT C Coding Standard
rule PRE11-C. Do not conclude macro definitions with a semicolon
(although the examples there are contrived).

Leaving the semicolon out also tends to confuse formatting tools.

It would be nice to be able to add a warning to help detect these
kinds of problems in macros (and others), and enable it for GCC
itself.
Jakub Jelinek March 20, 2017, 4:53 p.m.
On Mon, Mar 20, 2017 at 10:44:25AM -0600, Martin Sebor wrote:
> > /home/marek/src/gcc/gcc/sanitizer.def:459:1: note: in expansion of macro ‘DEF_SANITIZER_BUILTIN’
> >  DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH_ABORT
> 
> I would view these as helpful errors and expect them to be fixed
> by terminating the macro invocations with a semicolon rather than
> by adding it to the macro definition itself.  Is there a problem
> with doing that that I'm not considering?

There is a problem with it, as I wrote.
The macros are used e.g. like:
#define DEF_BUILTIN(ENUM, N, C, T, LT, B, F, NA, AT, IM, COND) ENUM,
enum built_in_function {
#include "builtins.def"
};
(similarly for the names various other cases).

If the *.def files contain the semicolon, then this is not possible.

	Jakub

Patch hide | download patch | download mbox

diff --git gcc/asan.c gcc/asan.c
index edcc6ea..a13679d 100644
--- gcc/asan.c
+++ gcc/asan.c
@@ -2567,10 +2567,12 @@  initialize_sanitizer_builtins (void)
 #define DEF_BUILTIN_STUB(ENUM, NAME)
 #undef DEF_SANITIZER_BUILTIN
 #define DEF_SANITIZER_BUILTIN(ENUM, NAME, TYPE, ATTRS) \
-  decl = add_builtin_function ("__builtin_" NAME, TYPE, ENUM,		\
-			       BUILT_IN_NORMAL, NAME, NULL_TREE);	\
-  set_call_expr_flags (decl, ATTRS);					\
-  set_builtin_decl (ENUM, decl, true);
+  do {									\
+    decl = add_builtin_function ("__builtin_" NAME, TYPE, ENUM,		\
+				 BUILT_IN_NORMAL, NAME, NULL_TREE);	\
+    set_call_expr_flags (decl, ATTRS);					\
+    set_builtin_decl (ENUM, decl, true);				\
+  } while (0);
 
 #include "sanitizer.def"