| Message ID | 20170320132802.GH3172@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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
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"
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