C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Marek Polacek]

A motivating example for this warning can be found e.g. in

  PRE10-C. Wrap multistatement macros in a do-while loop
  https://www.securecoding.cert.org/confluence/x/jgL7

i.e., 

#define SWAP(x, y) \
  tmp = x; \
  x = y; \
  y = tmp

used like this [1]

int x, y, z, tmp;
if (z == 0)
  SWAP(x, y);

expands to the following [2], which is certainly not what the programmer intended:

int x, y, z, tmp;
if (z == 0)
  tmp = x;
x = y;
y = tmp;

This has also happened in our codebase, see PR80063.

I tried to summarize the way I approached this problem in the commentary in
warn_for_multiline_expansion, but I'll try to explain the crux of the matter
here, too.

For code like [1], in the FEs we'll see [2], of course.  When parsing the
then-branch we see that the body of the if isn't wrapped in { } so we create a
compound statement with just the first statement "tmp = x;", and the other two
will be executed unconditionally.

My idea was to look at the location info of the following token after the body
of the if has been parsed and determine if they come from the same macro expansion,
and if they do (and the if itself doesn't), warn (taking into account various
corner cases, as usually).

For this I had to dive into line_maps, macro maps, etc., so CCing David to check
if my understanding of that is reasonable (hadn't worked with them before).

I've included this warning in -Wall, because there should be no false positives
(fingers crossed) and for most cases the warning should be pretty cheap.

I probably should've added a fix-it hint for good measure, too ("you better wrap
the damn macro in do {} while (0)"), but that can be done as a follow-up.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-06-01  Marek Polacek  <polacek@redhat.com>

	PR c/80116
	* c-common.h (warn_for_multiline_expansion): Declare.
	* c-warn.c (warn_for_multiline_expansion): New function.
	* c.opt (Wmultiline-expansion): New option.

	* c-parser.c (c_parser_if_body): Set the location of the
	body of the conditional after parsing all the labels.  Call
	warn_for_multiline_expansion.
	(c_parser_else_body): Likewise.

	* parser.c (cp_parser_statement): Add a default argument.  Save the
	location of the expression-statement after labels have been parsed.
	(cp_parser_implicitly_scoped_statement): Set the location of the
	body of the conditional after parsing all the labels.  Call
	warn_for_multiline_expansion.

	* doc/invoke.texi: Document -Wmultiline-expansion.

	* c-c++-common/Wmultiline-expansion-1.c: New test.
	* c-c++-common/Wmultiline-expansion-2.c: New test.
	* c-c++-common/Wmultiline-expansion-3.c: New test.
	* c-c++-common/Wmultiline-expansion-4.c: New test.
	* c-c++-common/Wmultiline-expansion-5.c: New test.
	* c-c++-common/Wmultiline-expansion-6.c: New test.

diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index 79072e6..6efbebc 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -1539,6 +1539,7 @@ extern bool maybe_warn_shift_overflow (location_t, tree, tree);
 extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **);
 extern bool diagnose_mismatched_attributes (tree, tree);
 extern tree do_warn_duplicated_branches_r (tree *, int *, void *);
+extern void warn_for_multiline_expansion (location_t, location_t, location_t);
 
 /* In c-attribs.c.  */
 extern bool attribute_takes_identifier_p (const_tree);
diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
index 012675b..16c6fc3 100644
--- gcc/c-family/c-warn.c
+++ gcc/c-family/c-warn.c
@@ -2392,3 +2392,105 @@ do_warn_duplicated_branches_r (tree *tp, int *, void *)
     do_warn_duplicated_branches (*tp);
   return NULL_TREE;
 }
+
+/* Implementation of -Wmultiline-expansion.  This warning warns about
+   cases when a macro expands to multiple statements not wrapped in
+   do {} while (0) or ({ }) and is used as a then branch or as an else
+   branch.  For example,
+
+   #define DOIT x++; y++
+
+   if (c)
+     DOIT;
+
+   will increment y unconditionally.
+
+   BODY_LOC is the location of the if/else body, NEXT_LOC is the location
+   of the next token after the if/else body has been parsed, and IF_LOC
+   is the location of the if condition or of the "else" keyword.  */
+
+void
+warn_for_multiline_expansion (location_t body_loc, location_t next_loc,
+			      location_t if_loc)
+{
+  if (!warn_multiline_expansion)
+    return;
+
+  /* Ain't got time to waste.  We only care about macros here.  */
+  if (!from_macro_expansion_at (body_loc)
+      || !from_macro_expansion_at (next_loc))
+    return;
+
+  /* Let's skip macros defined in system headers.  */
+  if (in_system_header_at (body_loc)
+      || in_system_header_at (next_loc))
+    return;
+
+  /* Find the actual tokens in the macro definition.  BODY_LOC and
+     NEXT_LOC have to come from the same spelling location, but they
+     will resolve to different locations in the context of the macro
+     definition.  */
+  location_t body_loc_exp
+    = linemap_resolve_location (line_table, body_loc,
+				LRK_MACRO_DEFINITION_LOCATION, NULL);
+  location_t next_loc_exp
+    = linemap_resolve_location (line_table, next_loc,
+				LRK_MACRO_DEFINITION_LOCATION, NULL);
+  location_t if_loc_exp
+    = linemap_resolve_location (line_table, if_loc,
+				LRK_MACRO_DEFINITION_LOCATION, NULL);
+
+  /* These are some funky cases we don't want to warn about.  */
+  if (body_loc_exp == if_loc_exp
+      || next_loc_exp == if_loc_exp
+      || body_loc_exp == next_loc_exp)
+    return;
+
+  /* Find the macro map for the macro expansion BODY_LOC.  Every
+     macro expansion gets its own macro map and we're looking for
+     the macro map containing the expansion of BODY_LOC.  We can't
+     simply check if BODY_LOC == MAP_START_LOCATION, because the
+     macro can start with a label, and then the BODY_LOC is a bit
+     offset farther into the map.  */
+  const line_map_macro *map = NULL;
+  for (const line_map_macro *cur_map = LINEMAPS_MACRO_MAPS (line_table);
+       cur_map && cur_map <= LINEMAPS_LAST_MACRO_MAP (line_table);
+       ++cur_map)
+    {
+      linemap_assert (linemap_macro_expansion_map_p (cur_map));
+      if (body_loc >= MAP_START_LOCATION (cur_map)
+	  && body_loc < (MAP_START_LOCATION (cur_map)
+			 + MACRO_MAP_NUM_MACRO_TOKENS (cur_map)))
+	map = cur_map;
+    }
+
+  /* We'd better find it.  */
+  gcc_assert (map != NULL);
+
+  /* Now see if the following token is coming from the same macro
+     expansion.  If it is, it's a problem, because it should've been
+     parsed at this point.
+
+     We're only looking at yN (i.e., the spelling locations) in
+     line_map_macro->macro_locations.  */
+  bool found_if = false;
+  bool found_next = false;
+  for (unsigned int i = 1;
+       i < 2 * MACRO_MAP_NUM_MACRO_TOKENS (map);
+       i += 2)
+    {
+      if (MACRO_MAP_LOCATIONS (map)[i] == next_loc_exp)
+	found_next = true;
+      if (MACRO_MAP_LOCATIONS (map)[i] == if_loc_exp)
+	found_if = true;
+    }
+
+  /* The if/else itself must not come from the same expansion, because
+     we don't want to warn about
+     #define IF if (x) x++; y++
+     and similar.  */
+  if (found_next && !found_if
+      && warning_at (body_loc, OPT_Wmultiline_expansion,
+		     "multiline macro expansion"))
+    inform (if_loc, "statement not guarded by this conditional");
+}
diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index a8543de..15a2681 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -698,6 +698,10 @@ Wmissing-field-initializers
 C ObjC C++ ObjC++ Var(warn_missing_field_initializers) Warning EnabledBy(Wextra)
 Warn about missing fields in struct initializers.
 
+Wmultiline-expansion
+C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn about macros expanding to multiple statements in a body of a conditional.
+
 Wmultiple-inheritance
 C++ ObjC++ Var(warn_multiple_inheritance) Warning
 Warn on direct multiple inheritance.
diff --git gcc/c/c-parser.c gcc/c/c-parser.c
index 03c711b..9460e51 100644
--- gcc/c/c-parser.c
+++ gcc/c/c-parser.c
@@ -5492,6 +5492,7 @@ c_parser_if_body (c_parser *parser, bool *if_p,
 {
   tree block = c_begin_compound_stmt (flag_isoc99);
   location_t body_loc = c_parser_peek_token (parser)->location;
+  location_t body_loc_after_labels = UNKNOWN_LOCATION;
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
 
@@ -5508,11 +5509,18 @@ c_parser_if_body (c_parser *parser, bool *if_p,
   else if (c_parser_next_token_is (parser, CPP_OPEN_BRACE))
     add_stmt (c_parser_compound_statement (parser));
   else
-    c_parser_statement_after_labels (parser, if_p);
+    {
+      body_loc_after_labels = c_parser_peek_token (parser)->location;
+      c_parser_statement_after_labels (parser, if_p);
+    }
 
   token_indent_info next_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (if_tinfo, body_tinfo, next_tinfo);
+  if (body_loc_after_labels != UNKNOWN_LOCATION
+      && next_tinfo.type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (body_loc_after_labels, next_tinfo.location,
+				  if_tinfo.location);
 
   return c_end_compound_stmt (body_loc, block, flag_isoc99);
 }
@@ -5530,6 +5538,7 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo,
   tree block = c_begin_compound_stmt (flag_isoc99);
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
+  location_t body_loc_after_labels = UNKNOWN_LOCATION;
 
   c_parser_all_labels (parser);
   if (c_parser_next_token_is (parser, CPP_SEMICOLON))
@@ -5542,11 +5551,18 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo,
       c_parser_consume_token (parser);
     }
   else
-    c_parser_statement_after_labels (parser, NULL, chain);
+    {
+      body_loc_after_labels = c_parser_peek_token (parser)->location;
+      c_parser_statement_after_labels (parser, NULL, chain);
+    }
 
   token_indent_info next_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (else_tinfo, body_tinfo, next_tinfo);
+  if (body_loc_after_labels != UNKNOWN_LOCATION
+      && next_tinfo.type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (body_loc_after_labels, next_tinfo.location,
+				  else_tinfo.location);
 
   return c_end_compound_stmt (body_loc, block, flag_isoc99);
 }
diff --git gcc/cp/parser.c gcc/cp/parser.c
index 313eebb..7eb8746 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -2102,7 +2102,7 @@ static void cp_parser_lambda_body
 /* Statements [gram.stmt.stmt]  */
 
 static void cp_parser_statement
-  (cp_parser *, tree, bool, bool *, vec<tree> * = NULL);
+  (cp_parser *, tree, bool, bool *, vec<tree> * = NULL, location_t * = NULL);
 static void cp_parser_label_for_labeled_statement
 (cp_parser *, tree);
 static tree cp_parser_expression_statement
@@ -10531,7 +10531,8 @@ cp_parser_lambda_body (cp_parser* parser, tree lambda_expr)
 
 static void
 cp_parser_statement (cp_parser* parser, tree in_statement_expr,
-		     bool in_compound, bool *if_p, vec<tree> *chain)
+		     bool in_compound, bool *if_p, vec<tree> *chain,
+		     location_t *loc_after_labels)
 {
   tree statement, std_attrs = NULL_TREE;
   cp_token *token;
@@ -10724,6 +10725,10 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
 	  if (cp_parser_parse_definitely (parser))
 	    return;
 	}
+      /* All preceding labels have been parsed at this point.  */
+      if (loc_after_labels != NULL)
+	*loc_after_labels = statement_location;
+
       /* Look for an expression-statement instead.  */
       statement = cp_parser_expression_statement (parser, in_statement_expr);
 
@@ -12264,6 +12269,7 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
 {
   tree statement;
   location_t body_loc = cp_lexer_peek_token (parser->lexer)->location;
+  location_t body_loc_after_labels = UNKNOWN_LOCATION;
   token_indent_info body_tinfo
     = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
 
@@ -12293,7 +12299,8 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
       /* Create a compound-statement.  */
       statement = begin_compound_stmt (0);
       /* Parse the dependent-statement.  */
-      cp_parser_statement (parser, NULL_TREE, false, if_p, chain);
+      cp_parser_statement (parser, NULL_TREE, false, if_p, chain,
+			   &body_loc_after_labels);
       /* Finish the dummy compound-statement.  */
       finish_compound_stmt (statement);
     }
@@ -12302,6 +12309,11 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
     = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
   warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo);
 
+  if (body_loc_after_labels != UNKNOWN_LOCATION
+      && next_tinfo.type != CPP_SEMICOLON)
+  warn_for_multiline_expansion (body_loc_after_labels, next_tinfo.location,
+				guard_tinfo.location);
+
   /* Return the statement.  */
   return statement;
 }
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 59563aa..ed59da4 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -292,7 +292,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wmain  -Wmaybe-uninitialized  -Wmemset-elt-size  -Wmemset-transposed-args @gol
 -Wmisleading-indentation  -Wmissing-braces @gol
 -Wmissing-field-initializers  -Wmissing-include-dirs @gol
--Wno-multichar  -Wnonnull  -Wnonnull-compare @gol
+-Wno-multichar  -Wmultiline-expansion  -Wnonnull  -Wnonnull-compare @gol
 -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol
 -Wnull-dereference  -Wodr  -Wno-overflow  -Wopenmp-simd  @gol
 -Woverride-init-side-effects  -Woverlength-strings @gol
@@ -3822,6 +3822,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}.
 -Wmemset-transposed-args @gol
 -Wmisleading-indentation @r{(only for C/C++)} @gol
 -Wmissing-braces @r{(only for C/ObjC)} @gol
+-Wmultiline-expansion  @gol
 -Wnarrowing @r{(only for C++)}  @gol
 -Wnonnull  @gol
 -Wnonnull-compare  @gol
@@ -4493,6 +4494,22 @@ This warning is enabled by @option{-Wall}.
 @opindex Wno-missing-include-dirs
 Warn if a user-supplied include directory does not exist.
 
+@item -Wmultiline-expansion
+@opindex Wmultiline-expansion
+@opindex Wno-multiline-expansion
+Warn about macros expanding to multiple statements in a body of a conditional.
+For example:
+
+@smallexample
+#define DOIT x++; y++
+if (c)
+  DOIT;
+@end smallexample
+
+will increment @code{y} unconditionally, not just when @code{c} holds.
+
+This warning is enabled by @option{-Wall} in C and C++.
+
 @item -Wparentheses
 @opindex Wparentheses
 @opindex Wno-parentheses
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
index e69de29..419cb5b 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
@@ -0,0 +1,118 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(X, Y)	\
+  tmp = X; /* { dg-warning "multiline macro expansion" } */ \
+  X = Y;		\
+  Y = tmp
+
+#define STUFF		\
+  if (0) x = y
+
+#define STUFF2		\
+  if (0) x = y; x++
+
+#define STUFF3		\
+  if (x) /* { dg-message "statement not guarded" } */ \
+    SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define SET(X, Y)	\
+  (X) = (Y)
+
+#define STUFF4		\
+  if (x)		\
+    SET(x, y);		\
+  SET(x, y)
+
+#define STUFF5		\
+  { tmp = x; x = y; }
+
+#define STUFF6		\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn1 (void)
+{
+  if (x) /* { dg-message "statement not guarded" } */
+    SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */
+}
+
+void
+fn2 (void)
+{
+  SWAP(x, y);
+}
+
+void
+fn3 (void)
+{
+  if (x)
+    {
+      SWAP(x, y);
+    }
+}
+
+void
+fn4 (void)
+{
+  if (x)
+  ({ x = 10; x++; });
+}
+
+void
+fn5 (void)
+{
+  if (x) /* { dg-message "statement not guarded" } */
+L1:
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+  goto L1;
+}
+
+void
+fn6 (void)
+{
+  if (x)
+    SET (x, y);
+  SET (tmp, x);
+}
+
+void
+fn7 (void)
+{
+  STUFF;
+}
+
+void
+fn8 (void)
+{
+  STUFF2;
+}
+
+void
+fn9 (void)
+{
+  STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */
+}
+
+void
+fn10 (void)
+{
+  STUFF4;
+}
+
+void
+fn11 (void)
+{
+  if (x)
+    STUFF5;
+}
+
+void
+fn12 (void)
+{
+  if (x)
+    STUFF6;
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
index e69de29..3a523d0 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
@@ -0,0 +1,137 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(X, Y)	\
+  tmp = X; /* { dg-warning "multiline macro expansion" } */ \
+  X = Y;		\
+  Y = tmp
+
+#define STUFF		\
+  if (0) {} else x = y
+
+#define STUFF2		\
+  if (0) {} else x = y; x++
+
+#define STUFF3		\
+  if (x)		\
+    {}			\
+  else /* { dg-message "statement not guarded" } */ \
+    SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define SET(X, Y)	\
+  (X) = (Y)
+
+#define STUFF4		\
+  if (x)		\
+    {}			\
+  else			\
+    SET(x, y);		\
+  SET(x, y)
+
+#define STUFF5		\
+  { tmp = x; x = y; }
+
+#define STUFF6		\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn1 (void)
+{
+  if (x)
+   {
+   }
+  else /* { dg-message "statement not guarded" } */
+    SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */
+}
+
+void
+fn2 (void)
+{
+  SWAP(x, y);
+}
+
+void
+fn3 (void)
+{
+  if (x)
+    {
+    }
+  else
+    {
+      SWAP(x, y);
+    }
+}
+
+void
+fn4 (void)
+{
+  if (x)
+    {
+    }
+  else
+    ({ x = 10; x++; });
+}
+
+void
+fn5 (void)
+{
+  if (x)
+    {
+    }
+  else /* { dg-message "statement not guarded" } */
+L1:
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+  goto L1;
+}
+
+void
+fn6 (void)
+{
+  if (x)
+    {
+    }
+  else
+    SET (x, y);
+  SET (tmp, x);
+}
+
+void
+fn7 (void)
+{
+  STUFF;
+}
+
+void
+fn8 (void)
+{
+  STUFF2;
+}
+
+void
+fn9 (void)
+{
+  STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */
+}
+
+void
+fn10 (void)
+{
+  STUFF4;
+}
+
+void
+fn11 (void)
+{
+  if (x)
+    STUFF5;
+}
+
+void
+fn12 (void)
+{
+  if (x)
+    STUFF6;
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
index e69de29..c2e83af 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
@@ -0,0 +1,12 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define CHECK(X) if (!(X)) __builtin_abort ()
+
+void
+fn (int i)
+{
+  CHECK (i == 1);
+  CHECK (i == 2);
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
index e69de29..3ab76a7 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
@@ -0,0 +1,14 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define FN(C)		\
+  void			\
+  fn (void)		\
+  {			\
+    C;			\
+  }
+
+int i;
+
+FN (if (i) ++i)
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
index e69de29..95017ba 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
@@ -0,0 +1,18 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define M(N)	\
+L ## N:		\
+  x++; x++ /* { dg-warning "multiline macro expansion" } */
+
+int x, y, tmp;
+
+void
+fn1 (void)
+{
+  if (x) /* { dg-message "statement not guarded" } */
+   M (0); /* { dg-message "in expansion of macro .M." } */
+  if (x) /* { dg-message "statement not guarded" } */
+   M (1); /* { dg-message "in expansion of macro .M." } */
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
index e69de29..9f799e1 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
@@ -0,0 +1,22 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define M \
+  if (x) x++; x++
+
+void
+f (int x)
+{
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+}

	Marek

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[David Malcolm]

On Thu, 2017-06-01 at 18:45 +0200, Marek Polacek wrote:
> A motivating example for this warning can be found e.g. in
> 
>   PRE10-C. Wrap multistatement macros in a do-while loop
>   https://www.securecoding.cert.org/confluence/x/jgL7
> 
> i.e., 
> 
> #define SWAP(x, y) \
>   tmp = x; \
>   x = y; \
>   y = tmp
> 
> used like this [1]
> 
> int x, y, z, tmp;
> if (z == 0)
>   SWAP(x, y);
> 
> expands to the following [2], which is certainly not what the
> programmer intended:
> 
> int x, y, z, tmp;
> if (z == 0)
>   tmp = x;
> x = y;
> y = tmp;
> 
> This has also happened in our codebase, see PR80063.

The warning looks like a good idea.

This reminds me a lot of -Wmisleading-indentation.  Does that fire for
any of the cases?

The patch appears to only consider "if" and "else" clauses.  Shouldn't
it also cover "for", "while" and "do/while"?

> I tried to summarize the way I approached this problem in the
> commentary in
> warn_for_multiline_expansion, but I'll try to explain the crux of the
> matter
> here, too.
> 
> For code like [1], in the FEs we'll see [2], of course.  When parsing
> the
> then-branch we see that the body of the if isn't wrapped in { } so we
> create a
> compound statement with just the first statement "tmp = x;", and the
> other two
> will be executed unconditionally.
> 
> My idea was to look at the location info of the following token after
> the body
> of the if has been parsed and determine if they come from the same
> macro expansion,
> and if they do (and the if itself doesn't), warn (taking into account
> various
> corner cases, as usually).
> 
> For this I had to dive into line_maps, macro maps, etc., so CCing
> David to check
> if my understanding of that is reasonable (hadn't worked with them
> before).

(am looking)

> I've included this warning in -Wall, because there should be no false
> positives
> (fingers crossed) and for most cases the warning should be pretty
> cheap.
> 
> I probably should've added a fix-it hint for good measure, too ("you
> better wrap
> the damn macro in do {} while (0)"), but that can be done as a follow
> -up.

That would be excellent, but might be fiddly.  The fix-it hint
machinery currently "avoids" macros.

See rich_location::reject_impossible_fixit, where we currently reject
source_location (aka location_t) values that are within macro maps,
putting the rich_location into a "something awkward is going on" mode
where it doesn't display fix-it hints.  It ought to work if you're sure
to use locations for the fixit that are within the line_map_ordinary
for the *definition* of the macro - so some care is required.


> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2017-06-01  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c/80116
> 	* c-common.h (warn_for_multiline_expansion): Declare.
> 	* c-warn.c (warn_for_multiline_expansion): New function.
> 	* c.opt (Wmultiline-expansion): New option.
> 
> 	* c-parser.c (c_parser_if_body): Set the location of the
> 	body of the conditional after parsing all the labels.  Call
> 	warn_for_multiline_expansion.
> 	(c_parser_else_body): Likewise.
> 
> 	* parser.c (cp_parser_statement): Add a default argument.  Save
> the
> 	location of the expression-statement after labels have been
> parsed.
> 	(cp_parser_implicitly_scoped_statement): Set the location of
> the
> 	body of the conditional after parsing all the labels.  Call
> 	warn_for_multiline_expansion.
> 
> 	* doc/invoke.texi: Document -Wmultiline-expansion.
> 
> 	* c-c++-common/Wmultiline-expansion-1.c: New test.
> 	* c-c++-common/Wmultiline-expansion-2.c: New test.
> 	* c-c++-common/Wmultiline-expansion-3.c: New test.
> 	* c-c++-common/Wmultiline-expansion-4.c: New test.
> 	* c-c++-common/Wmultiline-expansion-5.c: New test.
> 	* c-c++-common/Wmultiline-expansion-6.c: New test.
> 
> diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
> index 79072e6..6efbebc 100644
> --- gcc/c-family/c-common.h
> +++ gcc/c-family/c-common.h
> @@ -1539,6 +1539,7 @@ extern bool maybe_warn_shift_overflow
> (location_t, tree, tree);
>  extern void warn_duplicated_cond_add_or_warn (location_t, tree,
> vec<tree> **);
>  extern bool diagnose_mismatched_attributes (tree, tree);
>  extern tree do_warn_duplicated_branches_r (tree *, int *, void *);
> +extern void warn_for_multiline_expansion (location_t, location_t,
> location_t);
>  
>  /* In c-attribs.c.  */
>  extern bool attribute_takes_identifier_p (const_tree);
> diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
> index 012675b..16c6fc3 100644
> --- gcc/c-family/c-warn.c
> +++ gcc/c-family/c-warn.c
> @@ -2392,3 +2392,105 @@ do_warn_duplicated_branches_r (tree *tp, int
> *, void *)
>      do_warn_duplicated_branches (*tp);
>    return NULL_TREE;
>  }
> +
> +/* Implementation of -Wmultiline-expansion.  This warning warns
> about

Is the name of the warning correct?  Shouldn't the warning be about
multiple *statement* macros, rather than multi-line macros?  The user
could have written:

#define SWAP(x, y) tmp = x; x = y; y = tmp

which is all on one line.  (and is terrible code, yes).

> +   cases when a macro expands to multiple statements not wrapped in
> +   do {} while (0) or ({ }) and is used as a then branch or as an
> else
> +   branch.  For example,
> +
> +   #define DOIT x++; y++
> +
> +   if (c)
> +     DOIT;
> +
> +   will increment y unconditionally.
> +
> +   BODY_LOC is the location of the if/else body, NEXT_LOC is the
> location
> +   of the next token after the if/else body has been parsed, and
> IF_LOC
> +   is the location of the if condition or of the "else" keyword.  */
> +
> +void
> +warn_for_multiline_expansion (location_t body_loc, location_t
> next_loc,
> +			      location_t if_loc)

Should "if_loc" be "guard_loc"?
(and cover while, for, do)

> +{
> +  if (!warn_multiline_expansion)
> +    return;
> +
> +  /* Ain't got time to waste.  We only care about macros here.  */
> +  if (!from_macro_expansion_at (body_loc)
> +      || !from_macro_expansion_at (next_loc))
> +    return;
> +
> +  /* Let's skip macros defined in system headers.  */
> +  if (in_system_header_at (body_loc)
> +      || in_system_header_at (next_loc))
> +    return;
> +
> +  /* Find the actual tokens in the macro definition.  BODY_LOC and
> +     NEXT_LOC have to come from the same spelling location, but they
> +     will resolve to different locations in the context of the macro
> +     definition.  */
> +  location_t body_loc_exp
> +    = linemap_resolve_location (line_table, body_loc,
> +				LRK_MACRO_DEFINITION_LOCATION,
> NULL);
> +  location_t next_loc_exp
> +    = linemap_resolve_location (line_table, next_loc,
> +				LRK_MACRO_DEFINITION_LOCATION,
> NULL);
> +  location_t if_loc_exp
> +    = linemap_resolve_location (line_table, if_loc,
> +				LRK_MACRO_DEFINITION_LOCATION,
> NULL);
> +  /* These are some funky cases we don't want to warn about.  */
> +  if (body_loc_exp == if_loc_exp

What if they are all in one macro expansion, e.g.:

#define BODY_AND_IF(COND, X, Y)   if (COND) SWAP (X, Y);

Then

   BODY_AND_IF(some_flag, x, y)

would (I believe) expand to:

  if (some_flag)
    tmp = x;
  x = y;
  y = tmp;

which merits a warning.


> +      || next_loc_exp == if_loc_exp
> +      || body_loc_exp == next_loc_exp)
> +    return;


> +  /* Find the macro map for the macro expansion BODY_LOC.  Every
> +     macro expansion gets its own macro map and we're looking for
> +     the macro map containing the expansion of BODY_LOC.  We can't
> +     simply check if BODY_LOC == MAP_START_LOCATION, because the
> +     macro can start with a label, and then the BODY_LOC is a bit
> +     offset farther into the map.  */
> +  const line_map_macro *map = NULL;

Suggest renaming "map" to "macro_map" to emphasize that it's a macro
expansion.

> +  for (const line_map_macro *cur_map = LINEMAPS_MACRO_MAPS
> (line_table);
> +       cur_map && cur_map <= LINEMAPS_LAST_MACRO_MAP (line_table);
> +       ++cur_map)
> +    {
> +      linemap_assert (linemap_macro_expansion_map_p (cur_map));
> +      if (body_loc >= MAP_START_LOCATION (cur_map)
> +	  && body_loc < (MAP_START_LOCATION (cur_map)
> +			 + MACRO_MAP_NUM_MACRO_TOKENS (cur_map)))
> +	map = cur_map;
> +    }

I believe you can use linemap_macro_map_lookup for this, which does a
binary search through the macro maps, with caching, rather than a
linear one.

> +  /* We'd better find it.  */
> +  gcc_assert (map != NULL);

I was wondering if this needs to be defensively coded, rather than an
assert but given the
  from_macro_expansion_at (body_loc)
above, I don't see a way for map to be non-NULL.

> +  /* Now see if the following token is coming from the same macro
> +     expansion.  If it is, it's a problem, because it should've been
> +     parsed at this point.
> +
> +     We're only looking at yN (i.e., the spelling locations) in
> +     line_map_macro->macro_locations.  */

"and hence we only look at odd-numbered indexes within the
 MACRO_MAP_LOCATIONS array" or somesuch.

AIUI, this is looking at the spelling locations of the tokens within
the *definition* of the macro, so e.g. for 

#define SWAP(x, y) \
   tmp = x; \
   x = y; \
   y = tmp 

int a, b, c, tmp;
if (c == 0)
   SWAP(a, b);

expanded to:

int a, b, c, tmp;
if (c == 0)  <== IF_LOC
   tmp = a;  <== BODY_LOC, within macro map
                           x = loc of "a" in SWAP usage
                           y = loc of "tmp = x" in SWAP defn
   a = b;    <== NEXT_LOC, within macro map
                           x = loc of "b" in SWAP usage
                           y = loc of "x = y" in SWAP defn
   b = tmp;

though I'd have to check.

> +  bool found_if = false;
> +  bool found_next = false;
> +  for (unsigned int i = 1;
> +       i < 2 * MACRO_MAP_NUM_MACRO_TOKENS (map);
> +       i += 2)
> +    {
> +      if (MACRO_MAP_LOCATIONS (map)[i] == next_loc_exp)
> +	found_next = true;
> +      if (MACRO_MAP_LOCATIONS (map)[i] == if_loc_exp)
> +	found_if = true;
> +    }

So it looks like you're checking if NEXT_LOC_EXP and IF_LOC_EXP come
from the definition.

Is there any chance that we're not dealing with tokens here?  (I was
worried that we might be dealing with a location from an expression). 
 The leading comment says "NEXT_LOC is the location of the next token";
should the description of the rest of the arguments clarify that they
too are locations of tokens?


> +  /* The if/else itself must not come from the same expansion,
> because
> +     we don't want to warn about
> +     #define IF if (x) x++; y++
> +     and similar.  */
> +  if (found_next && !found_if
> +      && warning_at (body_loc, OPT_Wmultiline_expansion,
> +		     "multiline macro expansion"))
> +    inform (if_loc, "statement not guarded by this conditional");

IMHO this would be clearer if you restructure this final set of
conditionals into a rejection conditional, and then the warning as a
separate conditional; something like:

if (!found_next)
  return;
if (found_if)
  return;

/* Passed all tests.  */

if (warning_at (etc))
  inform (etc);



FWIW I strongly prefer the approach of
  if (!test_1)
    return
  if (!test_2)
    return;
  ...
  if (!test_N)
    return;

  do_something ();

over:

  if (test_1)
    if (test_2)
      ...
        if (test_N)
           do_something ();

since the latter coding style tends to push things over to the right
margin.


Nitpick about the messages; should it be something like:

warning_at (..., "macro expands to multiple statements without %<do {}
while (0)%> guard");

inform (..., "some parts of macro expansion are not guarded by this
<%if%>")

or somesuch.  Though, that said, how does it look to the end-user?




+      && warning_at (body_loc, OPT_Wmultiline_expansion,
> +		     "multiline macro expansion"))
> +    inform (if_loc, "statement not guarded by this conditional");

  


> diff --git gcc/c-family/c.opt gcc/c-family/c.opt
> index a8543de..15a2681 100644
> --- gcc/c-family/c.opt
> +++ gcc/c-family/c.opt
> @@ -698,6 +698,10 @@ Wmissing-field-initializers
>  C ObjC C++ ObjC++ Var(warn_missing_field_initializers) Warning
> EnabledBy(Wextra)
>  Warn about missing fields in struct initializers.
>  
> +Wmultiline-expansion
> +C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning
> LangEnabledBy(C ObjC C++ ObjC++,Wall)
> +Warn about macros expanding to multiple statements in a body of a
> conditional.

(earlier nitpick applies here).

>  Wmultiple-inheritance
>  C++ ObjC++ Var(warn_multiple_inheritance) Warning
>  Warn on direct multiple inheritance.

[...]

>  }
> diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
> index 59563aa..ed59da4 100644
> --- gcc/doc/invoke.texi
> +++ gcc/doc/invoke.texi
> @@ -292,7 +292,7 @@ Objective-C and Objective-C++ Dialects}.
>  -Wmain  -Wmaybe-uninitialized  -Wmemset-elt-size  -Wmemset
> -transposed-args @gol
>  -Wmisleading-indentation  -Wmissing-braces @gol
>  -Wmissing-field-initializers  -Wmissing-include-dirs @gol
> --Wno-multichar  -Wnonnull  -Wnonnull-compare @gol
> +-Wno-multichar  -Wmultiline-expansion  -Wnonnull  -Wnonnull-compare
> @gol
>  -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol
>  -Wnull-dereference  -Wodr  -Wno-overflow  -Wopenmp-simd  @gol
>  -Woverride-init-side-effects  -Woverlength-strings @gol
> @@ -3822,6 +3822,7 @@ Options} and @ref{Objective-C and Objective-C++
> Dialect Options}.
>  -Wmemset-transposed-args @gol
>  -Wmisleading-indentation @r{(only for C/C++)} @gol
>  -Wmissing-braces @r{(only for C/ObjC)} @gol
> +-Wmultiline-expansion  @gol
>  -Wnarrowing @r{(only for C++)}  @gol
>  -Wnonnull  @gol
>  -Wnonnull-compare  @gol
> @@ -4493,6 +4494,22 @@ This warning is enabled by @option{-Wall}.
>  @opindex Wno-missing-include-dirs
>  Warn if a user-supplied include directory does not exist.
>  
> +@item -Wmultiline-expansion
> +@opindex Wmultiline-expansion
> +@opindex Wno-multiline-expansion
> +Warn about macros expanding to multiple statements in a body of a
> conditional.
> +For example:
> +
> +@smallexample
> +#define DOIT x++; y++
> +if (c)
> +  DOIT;
> +@end smallexample
> +
> +will increment @code{y} unconditionally, not just when @code{c}
> holds.
> +
> +This warning is enabled by @option{-Wall} in C and C++.

Might be nice to mention the do-while workaround here.

>  @item -Wparentheses
>  @opindex Wparentheses
>  @opindex Wno-parentheses
> diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
> gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
> index e69de29..419cb5b 100644
> --- gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
> +++ gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
> @@ -0,0 +1,118 @@
> +/* PR c/80116 */
> +/* { dg-options "-Wmultiline-expansion" } */
> +/* { dg-do compile } */
> +
> +#define SWAP(X, Y)	\
> +  tmp = X; /* { dg-warning "multiline macro expansion" } */ \
> +  X = Y;		\
> +  Y = tmp
> +
> +#define STUFF		\
> +  if (0) x = y
> +
> +#define STUFF2		\
> +  if (0) x = y; x++
> +
> +#define STUFF3		\
> +  if (x) /* { dg-message "statement not guarded" } */ \
> +    SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */
> +
> +#define SET(X, Y)	\
> +  (X) = (Y)
> +
> +#define STUFF4		\
> +  if (x)		\
> +    SET(x, y);		\
> +  SET(x, y)
> +
> +#define STUFF5		\
> +  { tmp = x; x = y; }
> +
> +#define STUFF6		\
> +  x++;;
> +
> +int x, y, tmp;
> +
> +void
> +fn1 (void)
> +{
> +  if (x) /* { dg-message "statement not guarded" } */
> +    SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */
> +}
> +
> +void
> +fn2 (void)
> +{
> +  SWAP(x, y);
> +}
> +
> +void
> +fn3 (void)
> +{
> +  if (x)
> +    {
> +      SWAP(x, y);
> +    }
> +}
> +
> +void
> +fn4 (void)
> +{
> +  if (x)
> +  ({ x = 10; x++; });
> +}
> +
> +void
> +fn5 (void)
> +{
> +  if (x) /* { dg-message "statement not guarded" } */
> +L1:
> +    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
> +  goto L1;
> +}
> +
> +void
> +fn6 (void)
> +{
> +  if (x)
> +    SET (x, y);
> +  SET (tmp, x);
> +}
> +
> +void
> +fn7 (void)
> +{
> +  STUFF;
> +}
> +
> +void
> +fn8 (void)
> +{
> +  STUFF2;
> +}
> +
> +void
> +fn9 (void)
> +{
> +  STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */
> +}
> +
> +void
> +fn10 (void)
> +{
> +  STUFF4;
> +}
> +
> +void
> +fn11 (void)
> +{
> +  if (x)
> +    STUFF5;
> +}
> +
> +void
> +fn12 (void)
> +{
> +  if (x)
> +    STUFF6;
> +}
> diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
> gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
> index e69de29..3a523d0 100644
> --- gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
> +++ gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
> @@ -0,0 +1,137 @@
> +/* PR c/80116 */
> +/* { dg-options "-Wmultiline-expansion" } */
> +/* { dg-do compile } */
> +
> +#define SWAP(X, Y)	\
> +  tmp = X; /* { dg-warning "multiline macro expansion" } */ \
> +  X = Y;		\
> +  Y = tmp
> +
> +#define STUFF		\
> +  if (0) {} else x = y
> +
> +#define STUFF2		\
> +  if (0) {} else x = y; x++
> +
> +#define STUFF3		\
> +  if (x)		\
> +    {}			\
> +  else /* { dg-message "statement not guarded" } */ \
> +    SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */
> +
> +#define SET(X, Y)	\
> +  (X) = (Y)
> +
> +#define STUFF4		\
> +  if (x)		\
> +    {}			\
> +  else			\
> +    SET(x, y);		\
> +  SET(x, y)
> +
> +#define STUFF5		\
> +  { tmp = x; x = y; }
> +
> +#define STUFF6		\
> +  x++;;
> +
> +int x, y, tmp;
> +
> +void
> +fn1 (void)
> +{
> +  if (x)
> +   {
> +   }
> +  else /* { dg-message "statement not guarded" } */
> +    SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */
> +}
> +
> +void
> +fn2 (void)
> +{
> +  SWAP(x, y);
> +}
> +
> +void
> +fn3 (void)
> +{
> +  if (x)
> +    {
> +    }
> +  else
> +    {
> +      SWAP(x, y);
> +    }
> +}
> +
> +void
> +fn4 (void)
> +{
> +  if (x)
> +    {
> +    }
> +  else
> +    ({ x = 10; x++; });
> +}
> +
> +void
> +fn5 (void)
> +{
> +  if (x)
> +    {
> +    }
> +  else /* { dg-message "statement not guarded" } */
> +L1:
> +    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
> +  goto L1;
> +}
> +
> +void
> +fn6 (void)
> +{
> +  if (x)
> +    {
> +    }
> +  else
> +    SET (x, y);
> +  SET (tmp, x);
> +}
> +
> +void
> +fn7 (void)
> +{
> +  STUFF;
> +}
> +
> +void
> +fn8 (void)
> +{
> +  STUFF2;
> +}
> +
> +void
> +fn9 (void)
> +{
> +  STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */
> +}
> +
> +void
> +fn10 (void)
> +{
> +  STUFF4;
> +}
> +
> +void
> +fn11 (void)
> +{
> +  if (x)
> +    STUFF5;
> +}
> +
> +void
> +fn12 (void)
> +{
> +  if (x)
> +    STUFF6;
> +}
> diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
> gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
> index e69de29..c2e83af 100644
> --- gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
> +++ gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
> @@ -0,0 +1,12 @@
> +/* PR c/80116 */
> +/* { dg-options "-Wmultiline-expansion" } */
> +/* { dg-do compile } */
> +
> +#define CHECK(X) if (!(X)) __builtin_abort ()
> +
> +void
> +fn (int i)
> +{
> +  CHECK (i == 1);
> +  CHECK (i == 2);
> +}
> diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
> gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
> index e69de29..3ab76a7 100644
> --- gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
> +++ gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
> @@ -0,0 +1,14 @@
> +/* PR c/80116 */
> +/* { dg-options "-Wmultiline-expansion" } */
> +/* { dg-do compile } */
> +
> +#define FN(C)		\
> +  void			\
> +  fn (void)		\
> +  {			\
> +    C;			\
> +  }
> +
> +int i;
> +
> +FN (if (i) ++i)
> diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
> gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
> index e69de29..95017ba 100644
> --- gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
> +++ gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
> @@ -0,0 +1,18 @@
> +/* PR c/80116 */
> +/* { dg-options "-Wmultiline-expansion" } */
> +/* { dg-do compile } */
> +
> +#define M(N)	\
> +L ## N:		\
> +  x++; x++ /* { dg-warning "multiline macro expansion" } */
> +
> +int x, y, tmp;
> +
> +void
> +fn1 (void)
> +{
> +  if (x) /* { dg-message "statement not guarded" } */
> +   M (0); /* { dg-message "in expansion of macro .M." } */
> +  if (x) /* { dg-message "statement not guarded" } */
> +   M (1); /* { dg-message "in expansion of macro .M." } */
> +}
> diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
> gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
> index e69de29..9f799e1 100644
> --- gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
> +++ gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
> @@ -0,0 +1,22 @@
> +/* PR c/80116 */
> +/* { dg-options "-Wmultiline-expansion" } */
> +/* { dg-do compile } */
> +
> +#define M \
> +  if (x) x++; x++
> +
> +void
> +f (int x)
> +{
> +  M;
> +  M;
> +  M;
> +  M;
> +  M;
> +  M;
> +  M;
> +  M;
> +  M;
> +  M;
> +  M;
> +}

Should the testcases have a version of PR 80063 within them?


Thanks; hope this was constructive.

Dave

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Trevor Saunders]

On Thu, Jun 01, 2017 at 06:45:17PM +0200, Marek Polacek wrote:
> A motivating example for this warning can be found e.g. in
> 
>   PRE10-C. Wrap multistatement macros in a do-while loop
>   https://www.securecoding.cert.org/confluence/x/jgL7
> 
> i.e., 
> 
> #define SWAP(x, y) \
>   tmp = x; \
>   x = y; \
>   y = tmp
> 
> used like this [1]
> 
> int x, y, z, tmp;
> if (z == 0)
>   SWAP(x, y);
> 
> expands to the following [2], which is certainly not what the programmer intended:
> 
> int x, y, z, tmp;
> if (z == 0)
>   tmp = x;
> x = y;
> y = tmp;
> 
> This has also happened in our codebase, see PR80063.
> 
> I tried to summarize the way I approached this problem in the commentary in
> warn_for_multiline_expansion, but I'll try to explain the crux of the matter
> here, too.
> 
> For code like [1], in the FEs we'll see [2], of course.  When parsing the
> then-branch we see that the body of the if isn't wrapped in { } so we create a
> compound statement with just the first statement "tmp = x;", and the other two
> will be executed unconditionally.
> 
> My idea was to look at the location info of the following token after the body
> of the if has been parsed and determine if they come from the same macro expansion,
> and if they do (and the if itself doesn't), warn (taking into account various
> corner cases, as usually).

 especially given that its not easy to warn until a questionable macro
 is used dangerously it seems to me like it would be good to allow
 people to be defensive and get warnings for any unbraced blocks.  It
 clearly can't be enabled for gcc, but as the cert link demonstrates
 several common style guides require all blocks to be braced.

 Trev

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Joseph Myers]

On Thu, 1 Jun 2017, David Malcolm wrote:

> The patch appears to only consider "if" and "else" clauses.  Shouldn't
> it also cover "for", "while" and "do/while"?

do/while would normally get a syntax error in the problem cases.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Martin Sebor]

On 06/01/2017 10:45 AM, Marek Polacek wrote:
> A motivating example for this warning can be found e.g. in
>
>   PRE10-C. Wrap multistatement macros in a do-while loop
>   https://www.securecoding.cert.org/confluence/x/jgL7
>
> i.e.,
>
> #define SWAP(x, y) \
>   tmp = x; \
>   x = y; \
>   y = tmp
>
> used like this [1]
>
> int x, y, z, tmp;
> if (z == 0)
>   SWAP(x, y);
>
> expands to the following [2], which is certainly not what the programmer intended:
>
> int x, y, z, tmp;
> if (z == 0)
>   tmp = x;
> x = y;
> y = tmp;
>
> This has also happened in our codebase, see PR80063.
>
> I tried to summarize the way I approached this problem in the commentary in
> warn_for_multiline_expansion, but I'll try to explain the crux of the matter
> here, too.
>
> For code like [1], in the FEs we'll see [2], of course.  When parsing the
> then-branch we see that the body of the if isn't wrapped in { } so we create a
> compound statement with just the first statement "tmp = x;", and the other two
> will be executed unconditionally.
>
> My idea was to look at the location info of the following token after the body
> of the if has been parsed and determine if they come from the same macro expansion,
> and if they do (and the if itself doesn't), warn (taking into account various
> corner cases, as usually).

Very nice.  I think David already suggested handling other statements
besides if (do/while), so let me just add for and switch (as in:
'switch (1) case SWAP (i, j);')

The location in the warning look like it could be improved to extend
from just the first column to the whole macro argument but I don't
suppose that's under the direct control of your patch.

Besides the statements already mentioned above, here are a couple
of corner cases I noticed are not handled while playing with the
patch:

   define M(x) x

   int f (int i)
   {
     if (i)
       M (--i; --i);   // can this be handled?

     return i;
   }

and

   define M(x) x; x

   int f (int i)
   {
     if (i)
       M (--i; --i);   // seems like this should be handled

     return i;
   }

As an aside since it's outside the subset of the bigger problem
you chose to solve, there is a related issue with macros that
expand to an unparenthesized binary (and even some unary)
expression:

   #define sum(x, y) x + y

   int n = 2 * sum (3, 5);

I'm not very familiar with this area of the parser but I would
expect it to be relatively straightforward to extend your solution
to handle this problem as well.

>
> For this I had to dive into line_maps, macro maps, etc., so CCing David to check
> if my understanding of that is reasonable (hadn't worked with them before).
>
> I've included this warning in -Wall, because there should be no false positives
> (fingers crossed) and for most cases the warning should be pretty cheap.
>
> I probably should've added a fix-it hint for good measure, too ("you better wrap
> the damn macro in do {} while (0)"), but that can be done as a follow-up.

A hint I'm sure would be helpful to a lot of users.  One caveat
to be aware of is that wrapping an expression in a 'do { } while
(0)' is not a viable solution when the value of the last statement
is used.  In those cases, using the comma expression instead (in
parentheses) is often the way to go.  I'd expect determining which
to offer to be less than trivial.

Martin

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Marek Polacek]

On Thu, Jun 01, 2017 at 02:08:21PM -0400, David Malcolm wrote:
> On Thu, 2017-06-01 at 18:45 +0200, Marek Polacek wrote:
> > A motivating example for this warning can be found e.g. in
> > 
> >   PRE10-C. Wrap multistatement macros in a do-while loop
> >   https://www.securecoding.cert.org/confluence/x/jgL7
> > 
> > i.e., 
> > 
> > #define SWAP(x, y) \
> >   tmp = x; \
> >   x = y; \
> >   y = tmp
> > 
> > used like this [1]
> > 
> > int x, y, z, tmp;
> > if (z == 0)
> >   SWAP(x, y);
> > 
> > expands to the following [2], which is certainly not what the
> > programmer intended:
> > 
> > int x, y, z, tmp;
> > if (z == 0)
> >   tmp = x;
> > x = y;
> > y = tmp;
> > 
> > This has also happened in our codebase, see PR80063.
> 
> The warning looks like a good idea.

Thanks, I hope it will help detect sneaky bugs.

> This reminds me a lot of -Wmisleading-indentation.  Does that fire for
> any of the cases?

Only when I used -ftrack-macro-expansion=0.
 
> The patch appears to only consider "if" and "else" clauses.  Shouldn't
> it also cover "for", "while" and "do/while"?
 
It should cover "for" and "while".  My new version implements this.

> > I tried to summarize the way I approached this problem in the
> > commentary in
> > warn_for_multiline_expansion, but I'll try to explain the crux of the
> > matter
> > here, too.
> > 
> > For code like [1], in the FEs we'll see [2], of course.  When parsing
> > the
> > then-branch we see that the body of the if isn't wrapped in { } so we
> > create a
> > compound statement with just the first statement "tmp = x;", and the
> > other two
> > will be executed unconditionally.
> > 
> > My idea was to look at the location info of the following token after
> > the body
> > of the if has been parsed and determine if they come from the same
> > macro expansion,
> > and if they do (and the if itself doesn't), warn (taking into account
> > various
> > corner cases, as usually).
> > 
> > For this I had to dive into line_maps, macro maps, etc., so CCing
> > David to check
> > if my understanding of that is reasonable (hadn't worked with them
> > before).
> 
> (am looking)
> 
> > I've included this warning in -Wall, because there should be no false
> > positives
> > (fingers crossed) and for most cases the warning should be pretty
> > cheap.
> > 
> > I probably should've added a fix-it hint for good measure, too ("you
> > better wrap
> > the damn macro in do {} while (0)"), but that can be done as a follow
> > -up.
> 
> That would be excellent, but might be fiddly.  The fix-it hint
> machinery currently "avoids" macros.
> 
> See rich_location::reject_impossible_fixit, where we currently reject
> source_location (aka location_t) values that are within macro maps,
> putting the rich_location into a "something awkward is going on" mode
> where it doesn't display fix-it hints.  It ought to work if you're sure
> to use locations for the fixit that are within the line_map_ordinary
> for the *definition* of the macro - so some care is required.

Ouch.  Let's let it be for now.

> > Bootstrapped/regtested on x86_64-linux, ok for trunk?
> > 
> > 2017-06-01  Marek Polacek  <polacek@redhat.com>
> > 
> > 	PR c/80116
> > 	* c-common.h (warn_for_multiline_expansion): Declare.
> > 	* c-warn.c (warn_for_multiline_expansion): New function.
> > 	* c.opt (Wmultiline-expansion): New option.
> > 
> > 	* c-parser.c (c_parser_if_body): Set the location of the
> > 	body of the conditional after parsing all the labels.  Call
> > 	warn_for_multiline_expansion.
> > 	(c_parser_else_body): Likewise.
> > 
> > 	* parser.c (cp_parser_statement): Add a default argument.  Save
> > the
> > 	location of the expression-statement after labels have been
> > parsed.
> > 	(cp_parser_implicitly_scoped_statement): Set the location of
> > the
> > 	body of the conditional after parsing all the labels.  Call
> > 	warn_for_multiline_expansion.
> > 
> > 	* doc/invoke.texi: Document -Wmultiline-expansion.
> > 
> > 	* c-c++-common/Wmultiline-expansion-1.c: New test.
> > 	* c-c++-common/Wmultiline-expansion-2.c: New test.
> > 	* c-c++-common/Wmultiline-expansion-3.c: New test.
> > 	* c-c++-common/Wmultiline-expansion-4.c: New test.
> > 	* c-c++-common/Wmultiline-expansion-5.c: New test.
> > 	* c-c++-common/Wmultiline-expansion-6.c: New test.
> > 
> > diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
> > index 79072e6..6efbebc 100644
> > --- gcc/c-family/c-common.h
> > +++ gcc/c-family/c-common.h
> > @@ -1539,6 +1539,7 @@ extern bool maybe_warn_shift_overflow
> > (location_t, tree, tree);
> >  extern void warn_duplicated_cond_add_or_warn (location_t, tree,
> > vec<tree> **);
> >  extern bool diagnose_mismatched_attributes (tree, tree);
> >  extern tree do_warn_duplicated_branches_r (tree *, int *, void *);
> > +extern void warn_for_multiline_expansion (location_t, location_t,
> > location_t);
> >  
> >  /* In c-attribs.c.  */
> >  extern bool attribute_takes_identifier_p (const_tree);
> > diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
> > index 012675b..16c6fc3 100644
> > --- gcc/c-family/c-warn.c
> > +++ gcc/c-family/c-warn.c
> > @@ -2392,3 +2392,105 @@ do_warn_duplicated_branches_r (tree *tp, int
> > *, void *)
> >      do_warn_duplicated_branches (*tp);
> >    return NULL_TREE;
> >  }
> > +
> > +/* Implementation of -Wmultiline-expansion.  This warning warns
> > about
> 
> Is the name of the warning correct?  Shouldn't the warning be about
> multiple *statement* macros, rather than multi-line macros?  The user
> could have written:
> 
> #define SWAP(x, y) tmp = x; x = y; y = tmp
> 
> which is all on one line.  (and is terrible code, yes).
 
Yeah, by "multiline" I really mean "multistatement".  But I hate the
name "-Wmultistatement-expansion".  Can anybode come up with something better?

> > +   cases when a macro expands to multiple statements not wrapped in
> > +   do {} while (0) or ({ }) and is used as a then branch or as an
> > else
> > +   branch.  For example,
> > +
> > +   #define DOIT x++; y++
> > +
> > +   if (c)
> > +     DOIT;
> > +
> > +   will increment y unconditionally.
> > +
> > +   BODY_LOC is the location of the if/else body, NEXT_LOC is the
> > location
> > +   of the next token after the if/else body has been parsed, and
> > IF_LOC
> > +   is the location of the if condition or of the "else" keyword.  */
> > +
> > +void
> > +warn_for_multiline_expansion (location_t body_loc, location_t
> > next_loc,
> > +			      location_t if_loc)
> 
> Should "if_loc" be "guard_loc"?
> (and cover while, for, do)
 
Sure, changed.

> > +{
> > +  if (!warn_multiline_expansion)
> > +    return;
> > +
> > +  /* Ain't got time to waste.  We only care about macros here.  */
> > +  if (!from_macro_expansion_at (body_loc)
> > +      || !from_macro_expansion_at (next_loc))
> > +    return;
> > +
> > +  /* Let's skip macros defined in system headers.  */
> > +  if (in_system_header_at (body_loc)
> > +      || in_system_header_at (next_loc))
> > +    return;
> > +
> > +  /* Find the actual tokens in the macro definition.  BODY_LOC and
> > +     NEXT_LOC have to come from the same spelling location, but they
> > +     will resolve to different locations in the context of the macro
> > +     definition.  */
> > +  location_t body_loc_exp
> > +    = linemap_resolve_location (line_table, body_loc,
> > +				LRK_MACRO_DEFINITION_LOCATION,
> > NULL);
> > +  location_t next_loc_exp
> > +    = linemap_resolve_location (line_table, next_loc,
> > +				LRK_MACRO_DEFINITION_LOCATION,
> > NULL);
> > +  location_t if_loc_exp
> > +    = linemap_resolve_location (line_table, if_loc,
> > +				LRK_MACRO_DEFINITION_LOCATION,
> > NULL);
> > +  /* These are some funky cases we don't want to warn about.  */
> > +  if (body_loc_exp == if_loc_exp
> 
> What if they are all in one macro expansion, e.g.:
> 
> #define BODY_AND_IF(COND, X, Y)   if (COND) SWAP (X, Y);
> 
> Then
> 
>    BODY_AND_IF(some_flag, x, y)
> 
> would (I believe) expand to:
> 
>   if (some_flag)
>     tmp = x;
>   x = y;
>   y = tmp;
> 
> which merits a warning.

Yea, we warn for this case, as we should.  I've added a new test, although
it should've been covered by the previous tests, too.
 
> > +      || next_loc_exp == if_loc_exp
> > +      || body_loc_exp == next_loc_exp)
> > +    return;
> 
> 
> > +  /* Find the macro map for the macro expansion BODY_LOC.  Every
> > +     macro expansion gets its own macro map and we're looking for
> > +     the macro map containing the expansion of BODY_LOC.  We can't
> > +     simply check if BODY_LOC == MAP_START_LOCATION, because the
> > +     macro can start with a label, and then the BODY_LOC is a bit
> > +     offset farther into the map.  */
> > +  const line_map_macro *map = NULL;
> 
> Suggest renaming "map" to "macro_map" to emphasize that it's a macro
> expansion.

Done.

> > +  for (const line_map_macro *cur_map = LINEMAPS_MACRO_MAPS
> > (line_table);
> > +       cur_map && cur_map <= LINEMAPS_LAST_MACRO_MAP (line_table);
> > +       ++cur_map)
> > +    {
> > +      linemap_assert (linemap_macro_expansion_map_p (cur_map));
> > +      if (body_loc >= MAP_START_LOCATION (cur_map)
> > +	  && body_loc < (MAP_START_LOCATION (cur_map)
> > +			 + MACRO_MAP_NUM_MACRO_TOKENS (cur_map)))
> > +	map = cur_map;
> > +    }
> 
> I believe you can use linemap_macro_map_lookup for this, which does a
> binary search through the macro maps, with caching, rather than a
> linear one.

Well, I can't use linemap_macro_map_lookup (that is a static function), but
what I can do, is to use linemap_lookup + linemap_check_macro.

> > +  /* We'd better find it.  */
> > +  gcc_assert (map != NULL);
> 
> I was wondering if this needs to be defensively coded, rather than an
> assert but given the
>   from_macro_expansion_at (body_loc)
> above, I don't see a way for map to be non-NULL.

I think given the above, no need to bother here.

> > +  /* Now see if the following token is coming from the same macro
> > +     expansion.  If it is, it's a problem, because it should've been
> > +     parsed at this point.
> > +
> > +     We're only looking at yN (i.e., the spelling locations) in
> > +     line_map_macro->macro_locations.  */
> 
> "and hence we only look at odd-numbered indexes within the
>  MACRO_MAP_LOCATIONS array" or somesuch.

Ok, I've adjusted the comment here.

> AIUI, this is looking at the spelling locations of the tokens within
> the *definition* of the macro, so e.g. for 
> 
> #define SWAP(x, y) \
>    tmp = x; \
>    x = y; \
>    y = tmp 
> 
> int a, b, c, tmp;
> if (c == 0)
>    SWAP(a, b);
> 
> expanded to:
> 
> int a, b, c, tmp;
> if (c == 0)  <== IF_LOC
>    tmp = a;  <== BODY_LOC, within macro map
>                            x = loc of "a" in SWAP usage
>                            y = loc of "tmp = x" in SWAP defn
>    a = b;    <== NEXT_LOC, within macro map
>                            x = loc of "b" in SWAP usage
>                            y = loc of "x = y" in SWAP defn
>    b = tmp;
> 
> though I'd have to check.

I think you are correct.  Matches my thinking.

> > +  bool found_if = false;
> > +  bool found_next = false;
> > +  for (unsigned int i = 1;
> > +       i < 2 * MACRO_MAP_NUM_MACRO_TOKENS (map);
> > +       i += 2)
> > +    {
> > +      if (MACRO_MAP_LOCATIONS (map)[i] == next_loc_exp)
> > +	found_next = true;
> > +      if (MACRO_MAP_LOCATIONS (map)[i] == if_loc_exp)
> > +	found_if = true;
> > +    }
> 
> So it looks like you're checking if NEXT_LOC_EXP and IF_LOC_EXP come
> from the definition.
> 
> Is there any chance that we're not dealing with tokens here?  (I was
> worried that we might be dealing with a location from an expression). 

I'm not sure I follow.  But...

>  The leading comment says "NEXT_LOC is the location of the next token";
> should the description of the rest of the arguments clarify that they
> too are locations of tokens?

...I clarified the comment, I hope.  So far it's been working as expected.

> > +  /* The if/else itself must not come from the same expansion,
> > because
> > +     we don't want to warn about
> > +     #define IF if (x) x++; y++
> > +     and similar.  */
> > +  if (found_next && !found_if
> > +      && warning_at (body_loc, OPT_Wmultiline_expansion,
> > +		     "multiline macro expansion"))
> > +    inform (if_loc, "statement not guarded by this conditional");
> 
> IMHO this would be clearer if you restructure this final set of
> conditionals into a rejection conditional, and then the warning as a
> separate conditional; something like:
> 
> if (!found_next)
>   return;
> if (found_if)
>   return;
> 
> /* Passed all tests.  */
> 
> if (warning_at (etc))
>   inform (etc);

Ok, I've changed this, although in this case I could go either way.

> 
> 
> FWIW I strongly prefer the approach of
>   if (!test_1)
>     return
>   if (!test_2)
>     return;
>   ...
>   if (!test_N)
>     return;
> 
>   do_something ();
> 
> over:
> 
>   if (test_1)
>     if (test_2)
>       ...
>         if (test_N)
>            do_something ();
> 
> since the latter coding style tends to push things over to the right
> margin.

Yea, me too.
 
> Nitpick about the messages; should it be something like:
> 
> warning_at (..., "macro expands to multiple statements without %<do {}
> while (0)%> guard");

I've used this, but without the do while part.

> inform (..., "some parts of macro expansion are not guarded by this
> <%if%>")

And this too, but I think it should read "conditional" instead of "if".

> > +Wmultiline-expansion
> > +C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning
> > LangEnabledBy(C ObjC C++ ObjC++,Wall)
> > +Warn about macros expanding to multiple statements in a body of a
> > conditional.
> 
> (earlier nitpick applies here).

I've expanded a bit on the description.  Better name for the warning still
solicited ;).

> > +@item -Wmultiline-expansion
> > +@opindex Wmultiline-expansion
> > +@opindex Wno-multiline-expansion
> > +Warn about macros expanding to multiple statements in a body of a
> > conditional.
> > +For example:
> > +
> > +@smallexample
> > +#define DOIT x++; y++
> > +if (c)
> > +  DOIT;
> > +@end smallexample
> > +
> > +will increment @code{y} unconditionally, not just when @code{c}
> > holds.
> > +
> > +This warning is enabled by @option{-Wall} in C and C++.
> 
> Might be nice to mention the do-while workaround here.

Definitely.  Done along with a small example.

> Should the testcases have a version of PR 80063 within them?

I think not; I believe that one is well-covered by the other tests.

> Thanks; hope this was constructive.

Thanks for the review, it definitely made the patch much better!

This is the revised patch.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-06-02  Marek Polacek  <polacek@redhat.com>

	PR c/80116
	* c-common.h (warn_for_multiline_expansion): Declare.
	* c-warn.c (warn_for_multiline_expansion): New function.
	* c.opt (Wmultiline-expansion): New option.

	* c-parser.c (c_parser_if_body): Set the location of the
	body of the conditional after parsing all the labels.  Call
	warn_for_multiline_expansion.
	(c_parser_else_body): Likewise.
	(c_parser_while_statement): Likewise.
	(c_parser_for_statement): Likewise.
	(c_parser_statement): Add a default argument.  Save the location
	after labels have been parsed.
	(c_parser_c99_block_statement): Likewise.

	* parser.c (cp_parser_statement): Add a default argument.  Save the
	location of the expression-statement after labels have been parsed.
	(cp_parser_implicitly_scoped_statement): Set the location of the
	body of the conditional after parsing all the labels.  Call
	warn_for_multiline_expansion.
	(cp_parser_already_scoped_statement): Likewise.

	* doc/invoke.texi: Document -Wmultiline-expansion.

	* c-c++-common/Wmultiline-expansion-1.c: New test.
	* c-c++-common/Wmultiline-expansion-2.c: New test.
	* c-c++-common/Wmultiline-expansion-3.c: New test.
	* c-c++-common/Wmultiline-expansion-4.c: New test.
	* c-c++-common/Wmultiline-expansion-5.c: New test.
	* c-c++-common/Wmultiline-expansion-6.c: New test.
	* c-c++-common/Wmultiline-expansion-7.c: New test.
	* c-c++-common/Wmultiline-expansion-8.c: New test.
	* c-c++-common/Wmultiline-expansion-9.c: New test.

diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index 79072e6..6efbebc 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -1539,6 +1539,7 @@ extern bool maybe_warn_shift_overflow (location_t, tree, tree);
 extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **);
 extern bool diagnose_mismatched_attributes (tree, tree);
 extern tree do_warn_duplicated_branches_r (tree *, int *, void *);
+extern void warn_for_multiline_expansion (location_t, location_t, location_t);
 
 /* In c-attribs.c.  */
 extern bool attribute_takes_identifier_p (const_tree);
diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
index 012675b..3e807be 100644
--- gcc/c-family/c-warn.c
+++ gcc/c-family/c-warn.c
@@ -2392,3 +2392,91 @@ do_warn_duplicated_branches_r (tree *tp, int *, void *)
     do_warn_duplicated_branches (*tp);
   return NULL_TREE;
 }
+
+/* Implementation of -Wmultiline-expansion.  This warning warns about
+   cases when a macro expands to multiple statements not wrapped in
+   do {} while (0) or ({ }) and is used as a body of if/else/for/while
+   conditionals.  For example,
+
+   #define DOIT x++; y++
+
+   if (c)
+     DOIT;
+
+   will increment y unconditionally.
+
+   BODY_LOC is the location of the first token in the body after labels
+   have been parsed, NEXT_LOC is the location of the next token after the
+   body of the conditional has been parsed, and GUARD_LOC is the location
+   of the conditional.  */
+
+void
+warn_for_multiline_expansion (location_t body_loc, location_t next_loc,
+			      location_t guard_loc)
+{
+  if (!warn_multiline_expansion)
+    return;
+
+  /* Ain't got time to waste.  We only care about macros here.  */
+  if (!from_macro_expansion_at (body_loc)
+      || !from_macro_expansion_at (next_loc))
+    return;
+
+  /* Let's skip macros defined in system headers.  */
+  if (in_system_header_at (body_loc)
+      || in_system_header_at (next_loc))
+    return;
+
+  /* Find the actual tokens in the macro definition.  BODY_LOC and
+     NEXT_LOC have to come from the same spelling location, but they
+     will resolve to different locations in the context of the macro
+     definition.  */
+  location_t body_loc_exp
+    = linemap_resolve_location (line_table, body_loc,
+				LRK_MACRO_DEFINITION_LOCATION, NULL);
+  location_t next_loc_exp
+    = linemap_resolve_location (line_table, next_loc,
+				LRK_MACRO_DEFINITION_LOCATION, NULL);
+  location_t guard_loc_exp
+    = linemap_resolve_location (line_table, guard_loc,
+				LRK_MACRO_DEFINITION_LOCATION, NULL);
+
+  /* These are some funky cases we don't want to warn about.  */
+  if (body_loc_exp == guard_loc_exp
+      || next_loc_exp == guard_loc_exp
+      || body_loc_exp == next_loc_exp)
+    return;
+
+  /* Find the macro map for the macro expansion BODY_LOC.  */
+  const line_map *map = linemap_lookup (line_table, body_loc);
+  const line_map_macro *macro_map = linemap_check_macro (map);
+
+  /* Now see if the following token is coming from the same macro
+     expansion.  If it is, it's a problem, because it should've been
+     parsed at this point.  We only look at odd-numbered indexes
+     within the MACRO_MAP_LOCATIONS array, i.e. the spelling locations
+     of the tokens.  */
+  bool found_guard = false;
+  bool found_next = false;
+  for (unsigned int i = 1;
+       i < 2 * MACRO_MAP_NUM_MACRO_TOKENS (macro_map);
+       i += 2)
+    {
+      if (MACRO_MAP_LOCATIONS (macro_map)[i] == next_loc_exp)
+	found_next = true;
+      if (MACRO_MAP_LOCATIONS (macro_map)[i] == guard_loc_exp)
+	found_guard = true;
+    }
+
+  /* The conditional itself must not come from the same expansion, because
+     we don't want to warn about
+     #define IF if (x) x++; y++
+     and similar.  */
+  if (!found_next || found_guard)
+    return;
+
+  if (warning_at (body_loc, OPT_Wmultiline_expansion,
+		  "macro expands to multiple statements"))
+    inform (guard_loc, "some parts of macro expansion are not guarded by "
+	    "this conditional");
+}
diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 37bb236..1043615 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -698,6 +698,10 @@ Wmissing-field-initializers
 C ObjC C++ ObjC++ Var(warn_missing_field_initializers) Warning EnabledBy(Wextra)
 Warn about missing fields in struct initializers.
 
+Wmultiline-expansion
+C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn about macros expanding to multiple statements in a body of a conditional such as if, else, while, or for.
+
 Wmultiple-inheritance
 C++ ObjC++ Var(warn_multiple_inheritance) Warning
 Warn on direct multiple inheritance.
diff --git gcc/c/c-parser.c gcc/c/c-parser.c
index 6f954f2..83d2c86b 100644
--- gcc/c/c-parser.c
+++ gcc/c/c-parser.c
@@ -1218,9 +1218,11 @@ static void c_parser_initval (c_parser *, struct c_expr *,
 static tree c_parser_compound_statement (c_parser *);
 static void c_parser_compound_statement_nostart (c_parser *);
 static void c_parser_label (c_parser *);
-static void c_parser_statement (c_parser *, bool *);
+static void c_parser_statement (c_parser *, bool *, location_t * = NULL);
 static void c_parser_statement_after_labels (c_parser *, bool *,
 					     vec<tree> * = NULL);
+static tree c_parser_c99_block_statement (c_parser *, bool *,
+					  location_t * = NULL);
 static void c_parser_if_statement (c_parser *, bool *, vec<tree> *);
 static void c_parser_switch_statement (c_parser *, bool *);
 static void c_parser_while_statement (c_parser *, bool, bool *);
@@ -5204,9 +5206,11 @@ c_parser_label (c_parser *parser)
    implement -Wparentheses.  */
 
 static void
-c_parser_statement (c_parser *parser, bool *if_p)
+c_parser_statement (c_parser *parser, bool *if_p, location_t *loc_after_labels)
 {
   c_parser_all_labels (parser);
+  if (loc_after_labels)
+    *loc_after_labels = c_parser_peek_token (parser)->location;
   c_parser_statement_after_labels (parser, if_p, NULL);
 }
 
@@ -5466,11 +5470,12 @@ c_parser_paren_condition (c_parser *parser)
    implement -Wparentheses.  */
 
 static tree
-c_parser_c99_block_statement (c_parser *parser, bool *if_p)
+c_parser_c99_block_statement (c_parser *parser, bool *if_p,
+			      location_t *loc_after_labels)
 {
   tree block = c_begin_compound_stmt (flag_isoc99);
   location_t loc = c_parser_peek_token (parser)->location;
-  c_parser_statement (parser, if_p);
+  c_parser_statement (parser, if_p, loc_after_labels);
   return c_end_compound_stmt (loc, block, flag_isoc99);
 }
 
@@ -5492,6 +5497,7 @@ c_parser_if_body (c_parser *parser, bool *if_p,
 {
   tree block = c_begin_compound_stmt (flag_isoc99);
   location_t body_loc = c_parser_peek_token (parser)->location;
+  location_t body_loc_after_labels = UNKNOWN_LOCATION;
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
 
@@ -5508,11 +5514,18 @@ c_parser_if_body (c_parser *parser, bool *if_p,
   else if (c_parser_next_token_is (parser, CPP_OPEN_BRACE))
     add_stmt (c_parser_compound_statement (parser));
   else
-    c_parser_statement_after_labels (parser, if_p);
+    {
+      body_loc_after_labels = c_parser_peek_token (parser)->location;
+      c_parser_statement_after_labels (parser, if_p);
+    }
 
   token_indent_info next_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (if_tinfo, body_tinfo, next_tinfo);
+  if (body_loc_after_labels != UNKNOWN_LOCATION
+      && next_tinfo.type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (body_loc_after_labels, next_tinfo.location,
+				  if_tinfo.location);
 
   return c_end_compound_stmt (body_loc, block, flag_isoc99);
 }
@@ -5530,6 +5543,7 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo,
   tree block = c_begin_compound_stmt (flag_isoc99);
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
+  location_t body_loc_after_labels = UNKNOWN_LOCATION;
 
   c_parser_all_labels (parser);
   if (c_parser_next_token_is (parser, CPP_SEMICOLON))
@@ -5542,11 +5556,18 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo,
       c_parser_consume_token (parser);
     }
   else
-    c_parser_statement_after_labels (parser, NULL, chain);
+    {
+      body_loc_after_labels = c_parser_peek_token (parser)->location;
+      c_parser_statement_after_labels (parser, NULL, chain);
+    }
 
   token_indent_info next_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (else_tinfo, body_tinfo, next_tinfo);
+  if (body_loc_after_labels != UNKNOWN_LOCATION
+      && next_tinfo.type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (body_loc_after_labels, next_tinfo.location,
+				  else_tinfo.location);
 
   return c_end_compound_stmt (body_loc, block, flag_isoc99);
 }
@@ -5783,7 +5804,8 @@ c_parser_while_statement (c_parser *parser, bool ivdep, bool *if_p)
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
 
-  body = c_parser_c99_block_statement (parser, if_p);
+  location_t loc_after_labels;
+  body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels);
   c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true);
   add_stmt (c_end_compound_stmt (loc, block, flag_isoc99));
   c_parser_maybe_reclassify_token (parser);
@@ -5792,6 +5814,10 @@ c_parser_while_statement (c_parser *parser, bool ivdep, bool *if_p)
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (while_tinfo, body_tinfo, next_tinfo);
 
+  if (next_tinfo.type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (loc_after_labels, next_tinfo.location,
+				  while_tinfo.location);
+
   c_break_label = save_break;
   c_cont_label = save_cont;
 }
@@ -6076,7 +6102,8 @@ c_parser_for_statement (c_parser *parser, bool ivdep, bool *if_p)
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
 
-  body = c_parser_c99_block_statement (parser, if_p);
+  location_t loc_after_labels;
+  body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels);
 
   if (is_foreach_statement)
     objc_finish_foreach_loop (loc, object_expression, collection_expression, body, c_break_label, c_cont_label);
@@ -6089,6 +6116,10 @@ c_parser_for_statement (c_parser *parser, bool ivdep, bool *if_p)
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (for_tinfo, body_tinfo, next_tinfo);
 
+  if (next_tinfo.type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (loc_after_labels, next_tinfo.location,
+				  for_tinfo.location);
+
   c_break_label = save_break;
   c_cont_label = save_cont;
 }
diff --git gcc/cp/parser.c gcc/cp/parser.c
index 313eebb..60dfc48 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -2102,7 +2102,7 @@ static void cp_parser_lambda_body
 /* Statements [gram.stmt.stmt]  */
 
 static void cp_parser_statement
-  (cp_parser *, tree, bool, bool *, vec<tree> * = NULL);
+  (cp_parser *, tree, bool, bool *, vec<tree> * = NULL, location_t * = NULL);
 static void cp_parser_label_for_labeled_statement
 (cp_parser *, tree);
 static tree cp_parser_expression_statement
@@ -10531,7 +10531,8 @@ cp_parser_lambda_body (cp_parser* parser, tree lambda_expr)
 
 static void
 cp_parser_statement (cp_parser* parser, tree in_statement_expr,
-		     bool in_compound, bool *if_p, vec<tree> *chain)
+		     bool in_compound, bool *if_p, vec<tree> *chain,
+		     location_t *loc_after_labels)
 {
   tree statement, std_attrs = NULL_TREE;
   cp_token *token;
@@ -10724,6 +10725,10 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
 	  if (cp_parser_parse_definitely (parser))
 	    return;
 	}
+      /* All preceding labels have been parsed at this point.  */
+      if (loc_after_labels != NULL)
+	*loc_after_labels = statement_location;
+
       /* Look for an expression-statement instead.  */
       statement = cp_parser_expression_statement (parser, in_statement_expr);
 
@@ -12264,6 +12269,7 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
 {
   tree statement;
   location_t body_loc = cp_lexer_peek_token (parser->lexer)->location;
+  location_t body_loc_after_labels = UNKNOWN_LOCATION;
   token_indent_info body_tinfo
     = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
 
@@ -12293,7 +12299,8 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
       /* Create a compound-statement.  */
       statement = begin_compound_stmt (0);
       /* Parse the dependent-statement.  */
-      cp_parser_statement (parser, NULL_TREE, false, if_p, chain);
+      cp_parser_statement (parser, NULL_TREE, false, if_p, chain,
+			   &body_loc_after_labels);
       /* Finish the dummy compound-statement.  */
       finish_compound_stmt (statement);
     }
@@ -12302,6 +12309,11 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
     = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
   warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo);
 
+  if (body_loc_after_labels != UNKNOWN_LOCATION
+      && next_tinfo.type != CPP_SEMICOLON)
+  warn_for_multiline_expansion (body_loc_after_labels, next_tinfo.location,
+				guard_tinfo.location);
+
   /* Return the statement.  */
   return statement;
 }
@@ -12320,11 +12332,17 @@ cp_parser_already_scoped_statement (cp_parser* parser, bool *if_p,
     {
       token_indent_info body_tinfo
 	= get_token_indent_info (cp_lexer_peek_token (parser->lexer));
+      location_t loc_after_labels;
 
-      cp_parser_statement (parser, NULL_TREE, false, if_p);
+      cp_parser_statement (parser, NULL_TREE, false, if_p, NULL,
+			   &loc_after_labels);
       token_indent_info next_tinfo
 	= get_token_indent_info (cp_lexer_peek_token (parser->lexer));
       warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo);
+
+      if (next_tinfo.type != CPP_SEMICOLON)
+	warn_for_multiline_expansion (loc_after_labels, next_tinfo.location,
+				      guard_tinfo.location);
     }
   else
     {
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 819e800..4688058 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -292,7 +292,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wmain  -Wmaybe-uninitialized  -Wmemset-elt-size  -Wmemset-transposed-args @gol
 -Wmisleading-indentation  -Wmissing-braces @gol
 -Wmissing-field-initializers  -Wmissing-include-dirs @gol
--Wno-multichar  -Wnonnull  -Wnonnull-compare @gol
+-Wno-multichar  -Wmultiline-expansion  -Wnonnull  -Wnonnull-compare @gol
 -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol
 -Wnull-dereference  -Wodr  -Wno-overflow  -Wopenmp-simd  @gol
 -Woverride-init-side-effects  -Woverlength-strings @gol
@@ -3822,6 +3822,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}.
 -Wmemset-transposed-args @gol
 -Wmisleading-indentation @r{(only for C/C++)} @gol
 -Wmissing-braces @r{(only for C/ObjC)} @gol
+-Wmultiline-expansion  @gol
 -Wnarrowing @r{(only for C++)}  @gol
 -Wnonnull  @gol
 -Wnonnull-compare  @gol
@@ -4494,6 +4495,29 @@ This warning is enabled by @option{-Wall}.
 @opindex Wno-missing-include-dirs
 Warn if a user-supplied include directory does not exist.
 
+@item -Wmultiline-expansion
+@opindex Wmultiline-expansion
+@opindex Wno-multiline-expansion
+Warn about macros expanding to multiple statements in a body of a conditional,
+such as @code{if}, @code{else}, @code{for}, or @code{while}.
+For example:
+
+@smallexample
+#define DOIT x++; y++
+if (c)
+  DOIT;
+@end smallexample
+
+will increment @code{y} unconditionally, not just when @code{c} holds.
+The can usually be fixed by wrapping the macro in a do-while loop:
+@smallexample
+#define DOIT do @{ x++; y++; @} while (0)
+if (c)
+  DOIT;
+@end smallexample
+
+This warning is enabled by @option{-Wall} in C and C++.
+
 @item -Wparentheses
 @opindex Wparentheses
 @opindex Wno-parentheses
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
index e69de29..119d119 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
@@ -0,0 +1,118 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(X, Y)	\
+  tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \
+  X = Y;		\
+  Y = tmp
+
+#define STUFF		\
+  if (0) x = y
+
+#define STUFF2		\
+  if (0) x = y; x++
+
+#define STUFF3		\
+  if (x) /* { dg-message "not guarded" } */ \
+    SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define SET(X, Y)	\
+  (X) = (Y)
+
+#define STUFF4		\
+  if (x)		\
+    SET(x, y);		\
+  SET(x, y)
+
+#define STUFF5		\
+  { tmp = x; x = y; }
+
+#define STUFF6		\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn1 (void)
+{
+  if (x) /* { dg-message "not guarded" } */
+    SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */
+}
+
+void
+fn2 (void)
+{
+  SWAP(x, y);
+}
+
+void
+fn3 (void)
+{
+  if (x)
+    {
+      SWAP(x, y);
+    }
+}
+
+void
+fn4 (void)
+{
+  if (x)
+  ({ x = 10; x++; });
+}
+
+void
+fn5 (void)
+{
+  if (x) /* { dg-message "not guarded" } */
+L1:
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+  goto L1;
+}
+
+void
+fn6 (void)
+{
+  if (x)
+    SET (x, y);
+  SET (tmp, x);
+}
+
+void
+fn7 (void)
+{
+  STUFF;
+}
+
+void
+fn8 (void)
+{
+  STUFF2;
+}
+
+void
+fn9 (void)
+{
+  STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */
+}
+
+void
+fn10 (void)
+{
+  STUFF4;
+}
+
+void
+fn11 (void)
+{
+  if (x)
+    STUFF5;
+}
+
+void
+fn12 (void)
+{
+  if (x)
+    STUFF6;
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
index e69de29..b39d253 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
@@ -0,0 +1,137 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(X, Y)	\
+  tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \
+  X = Y;		\
+  Y = tmp
+
+#define STUFF		\
+  if (0) {} else x = y
+
+#define STUFF2		\
+  if (0) {} else x = y; x++
+
+#define STUFF3		\
+  if (x)		\
+    {}			\
+  else /* { dg-message "not guarded" } */ \
+    SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define SET(X, Y)	\
+  (X) = (Y)
+
+#define STUFF4		\
+  if (x)		\
+    {}			\
+  else			\
+    SET(x, y);		\
+  SET(x, y)
+
+#define STUFF5		\
+  { tmp = x; x = y; }
+
+#define STUFF6		\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn1 (void)
+{
+  if (x)
+   {
+   }
+  else /* { dg-message "not guarded" } */
+    SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */
+}
+
+void
+fn2 (void)
+{
+  SWAP(x, y);
+}
+
+void
+fn3 (void)
+{
+  if (x)
+    {
+    }
+  else
+    {
+      SWAP(x, y);
+    }
+}
+
+void
+fn4 (void)
+{
+  if (x)
+    {
+    }
+  else
+    ({ x = 10; x++; });
+}
+
+void
+fn5 (void)
+{
+  if (x)
+    {
+    }
+  else /* { dg-message "not guarded" } */
+L1:
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+  goto L1;
+}
+
+void
+fn6 (void)
+{
+  if (x)
+    {
+    }
+  else
+    SET (x, y);
+  SET (tmp, x);
+}
+
+void
+fn7 (void)
+{
+  STUFF;
+}
+
+void
+fn8 (void)
+{
+  STUFF2;
+}
+
+void
+fn9 (void)
+{
+  STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */
+}
+
+void
+fn10 (void)
+{
+  STUFF4;
+}
+
+void
+fn11 (void)
+{
+  if (x)
+    STUFF5;
+}
+
+void
+fn12 (void)
+{
+  if (x)
+    STUFF6;
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
index e69de29..c2e83af 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
@@ -0,0 +1,12 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define CHECK(X) if (!(X)) __builtin_abort ()
+
+void
+fn (int i)
+{
+  CHECK (i == 1);
+  CHECK (i == 2);
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
index e69de29..3ab76a7 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
@@ -0,0 +1,14 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define FN(C)		\
+  void			\
+  fn (void)		\
+  {			\
+    C;			\
+  }
+
+int i;
+
+FN (if (i) ++i)
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
index e69de29..01ba0de 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
@@ -0,0 +1,18 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define M(N)	\
+L ## N:		\
+  x++; x++ /* { dg-warning "macro expands to multiple statements" } */
+
+int x, y, tmp;
+
+void
+fn1 (void)
+{
+  if (x) /* { dg-message "not guarded" } */
+   M (0); /* { dg-message "in expansion of macro .M." } */
+  if (x) /* { dg-message "not guarded" } */
+   M (1); /* { dg-message "in expansion of macro .M." } */
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
index e69de29..9f799e1 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
@@ -0,0 +1,22 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define M \
+  if (x) x++; x++
+
+void
+f (int x)
+{
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-7.c gcc/testsuite/c-c++-common/Wmultiline-expansion-7.c
index e69de29..b561d43 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-7.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-7.c
@@ -0,0 +1,18 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(X, Y)      \
+  tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \
+  X = Y;                \
+  Y = tmp
+
+#define BODY_AND_IF(COND, X, Y)  \
+  if (COND) SWAP (X, Y) /* { dg-message "in expansion of macro .SWAP." } */
+
+void
+fn (int x, int y)
+{
+  int tmp;
+  BODY_AND_IF (1, x, y); /* { dg-message "in expansion of macro .BODY_AND_IF." } */
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-8.c gcc/testsuite/c-c++-common/Wmultiline-expansion-8.c
index e69de29..c5d8205 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-8.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-8.c
@@ -0,0 +1,64 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(x, y) \
+  tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \
+  x = y; \
+  y = tmp
+
+#define M1				\
+  for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */ \
+    SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define M2				\
+  for (i = 0; i < 1; ++i)		\
+    x++
+
+#define M3				\
+  for (i = 0; i < 1; ++i)		\
+    x++;;
+
+#define M4				\
+  for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */ \
+L1:					\
+    SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define INC	\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn0 (void)
+{
+  int i;
+  for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+
+  for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */
+L:
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+  goto L;
+}
+
+void
+fn1 (void)
+{
+  int i;
+  M1; /* { dg-message "in expansion of macro .M1." } */
+  M2;
+  M3;
+  M4; /* { dg-message "in expansion of macro .M4." } */
+  goto L1;
+}
+
+void
+fn2 (void)
+{
+  for (int i = 0; i < 1; ++i)
+    INC
+
+  for (int i = 0; i < 1; ++i)
+    ({ x = 10; x++; });
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-9.c gcc/testsuite/c-c++-common/Wmultiline-expansion-9.c
index e69de29..c503ade 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-9.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-9.c
@@ -0,0 +1,62 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(x, y) \
+  tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \
+  x = y; \
+  y = tmp
+
+#define M1	\
+  while (x) /* { dg-message "not guarded" } */ \
+    SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define M2	\
+  while (x)	\
+    x++
+
+#define M3	\
+  while (x)	\
+    x++;;
+
+#define M4	\
+  while (x) /* { dg-message "not guarded" } */ \
+L1:		\
+    SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define INC	\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn0 (void)
+{
+  while (x) /* { dg-message "not guarded" } */
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+
+  while (x) /* { dg-message "not guarded" } */
+L:
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+  goto L;
+}
+
+void
+fn1 (void)
+{
+  M1; /* { dg-message "in expansion of macro .M1." } */
+  M2;
+  M3;
+  M4; /* { dg-message "in expansion of macro .M4." } */
+  goto L1;
+}
+
+void
+fn2 (void)
+{
+  while (x)
+    INC
+
+  while (x)
+    ({ x = 10; x++; });
+}

	Marek

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Marek Polacek]

On Thu, Jun 01, 2017 at 10:13:01PM +0000, Joseph Myers wrote:
> On Thu, 1 Jun 2017, David Malcolm wrote:
> 
> > The patch appears to only consider "if" and "else" clauses.  Shouldn't
> > it also cover "for", "while" and "do/while"?
> 
> do/while would normally get a syntax error in the problem cases.

Indeed, I think there's no point in handling "do/while" for this warning.

	Marek

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Marek Polacek]

On Thu, Jun 01, 2017 at 04:17:24PM -0600, Martin Sebor wrote:
> Very nice.  I think David already suggested handling other statements
> besides if (do/while), so let me just add for and switch (as in:
> 'switch (1) case SWAP (i, j);')

How's that one problematic, though?

> The location in the warning look like it could be improved to extend
> from just the first column to the whole macro argument but I don't
> suppose that's under the direct control of your patch.

Well, for e.g. the "in expasion" message we have a good location range:
  SWAP
  ^~~~
so do you mean this?
tmp = x;
^
?  But yea, it's outside the scope of this patch.

> Besides the statements already mentioned above, here are a couple
> of corner cases I noticed are not handled while playing with the
> patch:
> 
>   define M(x) x
> 
>   int f (int i)
>   {
>     if (i)
>       M (--i; --i);   // can this be handled?
> 
>     return i;
>   }
> 
> and
> 
>   define M(x) x; x
> 
>   int f (int i)
>   {
>     if (i)
>       M (--i; --i);   // seems like this should be handled
> 
>     return i;
>   }

Hmm, I was hoping my patch would warn for both examples, but it doesn't.  I'll
have to get back to this a ponder more.

> As an aside since it's outside the subset of the bigger problem
> you chose to solve, there is a related issue with macros that
> expand to an unparenthesized binary (and even some unary)
> expression:
> 
>   #define sum(x, y) x + y
> 
>   int n = 2 * sum (3, 5);
> 
> I'm not very familiar with this area of the parser but I would
> expect it to be relatively straightforward to extend your solution
> to handle this problem as well.

It'd certainly be useful to warn here.  But it doesn't seem to be an
easy warning to implement, to me.  E.g. warning for

  int n = 2 + sum (3, 5);

would be annoying, I suspect.

> > For this I had to dive into line_maps, macro maps, etc., so CCing David to check
> > if my understanding of that is reasonable (hadn't worked with them before).
> > 
> > I've included this warning in -Wall, because there should be no false positives
> > (fingers crossed) and for most cases the warning should be pretty cheap.
> > 
> > I probably should've added a fix-it hint for good measure, too ("you better wrap
> > the damn macro in do {} while (0)"), but that can be done as a follow-up.
> 
> A hint I'm sure would be helpful to a lot of users.  One caveat
> to be aware of is that wrapping an expression in a 'do { } while
> (0)' is not a viable solution when the value of the last statement
> is used.  In those cases, using the comma expression instead (in
> parentheses) is often the way to go.  I'd expect determining which
> to offer to be less than trivial.

This didn't occur to me at all.  Well, I still think we could just suggest
adding "do while (0)" and get away with that.

Thanks for taking a look!

	Marek

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Martin Sebor]

On 06/02/2017 10:52 AM, Marek Polacek wrote:
> On Thu, Jun 01, 2017 at 04:17:24PM -0600, Martin Sebor wrote:
>> Very nice.  I think David already suggested handling other statements
>> besides if (do/while), so let me just add for and switch (as in:
>> 'switch (1) case SWAP (i, j);')
>
> How's that one problematic, though?

The same way as 'if (1) SWAP (i, j);' because it expands to

   switch (1) case 1: tmp = i;
   i = j;
   j = tmp;

(I had a typo there so maybe that obscured the problem.)

>
>> The location in the warning look like it could be improved to extend
>> from just the first column to the whole macro argument but I don't
>> suppose that's under the direct control of your patch.
>
> Well, for e.g. the "in expasion" message we have a good location range:
>   SWAP
>   ^~~~
> so do you mean this?
> tmp = x;
> ^

Yes.

> ?  But yea, it's outside the scope of this patch.
>
>> Besides the statements already mentioned above, here are a couple
>> of corner cases I noticed are not handled while playing with the
>> patch:
>>
>>   define M(x) x
>>
>>   int f (int i)
>>   {
>>     if (i)
>>       M (--i; --i);   // can this be handled?
>>
>>     return i;
>>   }
>>
>> and
>>
>>   define M(x) x; x
>>
>>   int f (int i)
>>   {
>>     if (i)
>>       M (--i; --i);   // seems like this should be handled
>>
>>     return i;
>>   }
>
> Hmm, I was hoping my patch would warn for both examples, but it doesn't.  I'll
> have to get back to this a ponder more.
>
>> As an aside since it's outside the subset of the bigger problem
>> you chose to solve, there is a related issue with macros that
>> expand to an unparenthesized binary (and even some unary)
>> expression:
>>
>>   #define sum(x, y) x + y
>>
>>   int n = 2 * sum (3, 5);
>>
>> I'm not very familiar with this area of the parser but I would
>> expect it to be relatively straightforward to extend your solution
>> to handle this problem as well.
>
> It'd certainly be useful to warn here.  But it doesn't seem to be an
> easy warning to implement, to me.  E.g. warning for
>
>   int n = 2 + sum (3, 5);
>
> would be annoying, I suspect.

Yes, it would be if the warning was only meant to trigger for
uses that change the meaning.  If it was meant to warn on unsafe
macro definitions instead it should be less so.  But that would
make it a different warning, and mean implementing it in the
preprocessor.  Hmm, I guess it's not as straightforward as it
seemed.

Martin

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Marek Polacek]

On Fri, Jun 02, 2017 at 03:50:12PM -0600, Martin Sebor wrote:
> On 06/02/2017 10:52 AM, Marek Polacek wrote:
> > On Thu, Jun 01, 2017 at 04:17:24PM -0600, Martin Sebor wrote:
> > > Very nice.  I think David already suggested handling other statements
> > > besides if (do/while), so let me just add for and switch (as in:
> > > 'switch (1) case SWAP (i, j);')
> > 
> > How's that one problematic, though?
> 
> The same way as 'if (1) SWAP (i, j);' because it expands to
> 
>   switch (1) case 1: tmp = i;
>   i = j;
>   j = tmp;
> 
> (I had a typo there so maybe that obscured the problem.)

Ah, ok, I can see now.  Actually the C++ FE would already warn with my patch,
but the C FE needed small changes to warn.

> > 
> > > The location in the warning look like it could be improved to extend
> > > from just the first column to the whole macro argument but I don't
> > > suppose that's under the direct control of your patch.
> > 
> > Well, for e.g. the "in expasion" message we have a good location range:
> >   SWAP
> >   ^~~~
> > so do you mean this?
> > tmp = x;
> > ^
> 
> Yes.
> 
> > ?  But yea, it's outside the scope of this patch.
> > 
> > > Besides the statements already mentioned above, here are a couple
> > > of corner cases I noticed are not handled while playing with the
> > > patch:
> > > 
> > >   define M(x) x
> > > 
> > >   int f (int i)
> > >   {
> > >     if (i)
> > >       M (--i; --i);   // can this be handled?
> > > 
> > >     return i;
> > >   }
> > > 
> > > and
> > > 
> > >   define M(x) x; x
> > > 
> > >   int f (int i)
> > >   {
> > >     if (i)
> > >       M (--i; --i);   // seems like this should be handled
> > > 
> > >     return i;
> > >   }
> > 
> > Hmm, I was hoping my patch would warn for both examples, but it doesn't.  I'll
> > have to get back to this a ponder more.

I still haven't looked into this.  Might not be easy so I think let's not
block the patch on this.

> > > As an aside since it's outside the subset of the bigger problem
> > > you chose to solve, there is a related issue with macros that
> > > expand to an unparenthesized binary (and even some unary)
> > > expression:
> > > 
> > >   #define sum(x, y) x + y
> > > 
> > >   int n = 2 * sum (3, 5);
> > > 
> > > I'm not very familiar with this area of the parser but I would
> > > expect it to be relatively straightforward to extend your solution
> > > to handle this problem as well.
> > 
> > It'd certainly be useful to warn here.  But it doesn't seem to be an
> > easy warning to implement, to me.  E.g. warning for
> > 
> >   int n = 2 + sum (3, 5);
> > 
> > would be annoying, I suspect.
> 
> Yes, it would be if the warning was only meant to trigger for
> uses that change the meaning.  If it was meant to warn on unsafe
> macro definitions instead it should be less so.  But that would
> make it a different warning, and mean implementing it in the
> preprocessor.  Hmm, I guess it's not as straightforward as it
> seemed.

Nothing ever is ;).

All right, here's the third version of this patch which also warns about
the switch case Martin pointed out above.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2017-06-07  Marek Polacek  <polacek@redhat.com>

	PR c/80116
	* c-common.h (warn_for_multiline_expansion): Declare.
	* c-warn.c (warn_for_multiline_expansion): New function.
	* c.opt (Wmultiline-expansion): New option.

	* c-parser.c (c_parser_if_body): Set the location of the
	body of the conditional after parsing all the labels.  Call
	warn_for_multiline_expansion.
	(c_parser_else_body): Likewise.
	(c_parser_switch_statement): Likewise.
	(c_parser_while_statement): Likewise.
	(c_parser_for_statement): Likewise.
	(c_parser_statement): Add a default argument.  Save the location
	after labels have been parsed.
	(c_parser_c99_block_statement): Likewise.

	* parser.c (cp_parser_statement): Add a default argument.  Save the
	location of the expression-statement after labels have been parsed.
	(cp_parser_implicitly_scoped_statement): Set the location of the
	body of the conditional after parsing all the labels.  Call
	warn_for_multiline_expansion.
	(cp_parser_already_scoped_statement): Likewise.

	* doc/invoke.texi: Document -Wmultiline-expansion.

	* c-c++-common/Wmultiline-expansion-1.c: New test.
	* c-c++-common/Wmultiline-expansion-2.c: New test.
	* c-c++-common/Wmultiline-expansion-3.c: New test.
	* c-c++-common/Wmultiline-expansion-4.c: New test.
	* c-c++-common/Wmultiline-expansion-5.c: New test.
	* c-c++-common/Wmultiline-expansion-6.c: New test.
	* c-c++-common/Wmultiline-expansion-7.c: New test.
	* c-c++-common/Wmultiline-expansion-8.c: New test.
	* c-c++-common/Wmultiline-expansion-9.c: New test.
	* c-c++-common/Wmultiline-expansion-10.c: New test.

diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index 79072e6..6efbebc 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -1539,6 +1539,7 @@ extern bool maybe_warn_shift_overflow (location_t, tree, tree);
 extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **);
 extern bool diagnose_mismatched_attributes (tree, tree);
 extern tree do_warn_duplicated_branches_r (tree *, int *, void *);
+extern void warn_for_multiline_expansion (location_t, location_t, location_t);
 
 /* In c-attribs.c.  */
 extern bool attribute_takes_identifier_p (const_tree);
diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
index 35321a6..84e1552 100644
--- gcc/c-family/c-warn.c
+++ gcc/c-family/c-warn.c
@@ -2401,3 +2401,91 @@ do_warn_duplicated_branches_r (tree *tp, int *, void *)
     do_warn_duplicated_branches (*tp);
   return NULL_TREE;
 }
+
+/* Implementation of -Wmultiline-expansion.  This warning warns about
+   cases when a macro expands to multiple statements not wrapped in
+   do {} while (0) or ({ }) and is used as a body of if/else/for/while
+   conditionals.  For example,
+
+   #define DOIT x++; y++
+
+   if (c)
+     DOIT;
+
+   will increment y unconditionally.
+
+   BODY_LOC is the location of the first token in the body after labels
+   have been parsed, NEXT_LOC is the location of the next token after the
+   body of the conditional has been parsed, and GUARD_LOC is the location
+   of the conditional.  */
+
+void
+warn_for_multiline_expansion (location_t body_loc, location_t next_loc,
+			      location_t guard_loc)
+{
+  if (!warn_multiline_expansion)
+    return;
+
+  /* Ain't got time to waste.  We only care about macros here.  */
+  if (!from_macro_expansion_at (body_loc)
+      || !from_macro_expansion_at (next_loc))
+    return;
+
+  /* Let's skip macros defined in system headers.  */
+  if (in_system_header_at (body_loc)
+      || in_system_header_at (next_loc))
+    return;
+
+  /* Find the actual tokens in the macro definition.  BODY_LOC and
+     NEXT_LOC have to come from the same spelling location, but they
+     will resolve to different locations in the context of the macro
+     definition.  */
+  location_t body_loc_exp
+    = linemap_resolve_location (line_table, body_loc,
+				LRK_MACRO_DEFINITION_LOCATION, NULL);
+  location_t next_loc_exp
+    = linemap_resolve_location (line_table, next_loc,
+				LRK_MACRO_DEFINITION_LOCATION, NULL);
+  location_t guard_loc_exp
+    = linemap_resolve_location (line_table, guard_loc,
+				LRK_MACRO_DEFINITION_LOCATION, NULL);
+
+  /* These are some funky cases we don't want to warn about.  */
+  if (body_loc_exp == guard_loc_exp
+      || next_loc_exp == guard_loc_exp
+      || body_loc_exp == next_loc_exp)
+    return;
+
+  /* Find the macro map for the macro expansion BODY_LOC.  */
+  const line_map *map = linemap_lookup (line_table, body_loc);
+  const line_map_macro *macro_map = linemap_check_macro (map);
+
+  /* Now see if the following token is coming from the same macro
+     expansion.  If it is, it's a problem, because it should've been
+     parsed at this point.  We only look at odd-numbered indexes
+     within the MACRO_MAP_LOCATIONS array, i.e. the spelling locations
+     of the tokens.  */
+  bool found_guard = false;
+  bool found_next = false;
+  for (unsigned int i = 1;
+       i < 2 * MACRO_MAP_NUM_MACRO_TOKENS (macro_map);
+       i += 2)
+    {
+      if (MACRO_MAP_LOCATIONS (macro_map)[i] == next_loc_exp)
+	found_next = true;
+      if (MACRO_MAP_LOCATIONS (macro_map)[i] == guard_loc_exp)
+	found_guard = true;
+    }
+
+  /* The conditional itself must not come from the same expansion, because
+     we don't want to warn about
+     #define IF if (x) x++; y++
+     and similar.  */
+  if (!found_next || found_guard)
+    return;
+
+  if (warning_at (body_loc, OPT_Wmultiline_expansion,
+		  "macro expands to multiple statements"))
+    inform (guard_loc, "some parts of macro expansion are not guarded by "
+	    "this conditional");
+}
diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 37bb236..1043615 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -698,6 +698,10 @@ Wmissing-field-initializers
 C ObjC C++ ObjC++ Var(warn_missing_field_initializers) Warning EnabledBy(Wextra)
 Warn about missing fields in struct initializers.
 
+Wmultiline-expansion
+C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn about macros expanding to multiple statements in a body of a conditional such as if, else, while, or for.
+
 Wmultiple-inheritance
 C++ ObjC++ Var(warn_multiple_inheritance) Warning
 Warn on direct multiple inheritance.
diff --git gcc/c/c-parser.c gcc/c/c-parser.c
index 6f954f2..d46f81c 100644
--- gcc/c/c-parser.c
+++ gcc/c/c-parser.c
@@ -1218,9 +1218,11 @@ static void c_parser_initval (c_parser *, struct c_expr *,
 static tree c_parser_compound_statement (c_parser *);
 static void c_parser_compound_statement_nostart (c_parser *);
 static void c_parser_label (c_parser *);
-static void c_parser_statement (c_parser *, bool *);
+static void c_parser_statement (c_parser *, bool *, location_t * = NULL);
 static void c_parser_statement_after_labels (c_parser *, bool *,
 					     vec<tree> * = NULL);
+static tree c_parser_c99_block_statement (c_parser *, bool *,
+					  location_t * = NULL);
 static void c_parser_if_statement (c_parser *, bool *, vec<tree> *);
 static void c_parser_switch_statement (c_parser *, bool *);
 static void c_parser_while_statement (c_parser *, bool, bool *);
@@ -5204,9 +5206,11 @@ c_parser_label (c_parser *parser)
    implement -Wparentheses.  */
 
 static void
-c_parser_statement (c_parser *parser, bool *if_p)
+c_parser_statement (c_parser *parser, bool *if_p, location_t *loc_after_labels)
 {
   c_parser_all_labels (parser);
+  if (loc_after_labels)
+    *loc_after_labels = c_parser_peek_token (parser)->location;
   c_parser_statement_after_labels (parser, if_p, NULL);
 }
 
@@ -5466,11 +5470,12 @@ c_parser_paren_condition (c_parser *parser)
    implement -Wparentheses.  */
 
 static tree
-c_parser_c99_block_statement (c_parser *parser, bool *if_p)
+c_parser_c99_block_statement (c_parser *parser, bool *if_p,
+			      location_t *loc_after_labels)
 {
   tree block = c_begin_compound_stmt (flag_isoc99);
   location_t loc = c_parser_peek_token (parser)->location;
-  c_parser_statement (parser, if_p);
+  c_parser_statement (parser, if_p, loc_after_labels);
   return c_end_compound_stmt (loc, block, flag_isoc99);
 }
 
@@ -5492,6 +5497,7 @@ c_parser_if_body (c_parser *parser, bool *if_p,
 {
   tree block = c_begin_compound_stmt (flag_isoc99);
   location_t body_loc = c_parser_peek_token (parser)->location;
+  location_t body_loc_after_labels = UNKNOWN_LOCATION;
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
 
@@ -5508,11 +5514,18 @@ c_parser_if_body (c_parser *parser, bool *if_p,
   else if (c_parser_next_token_is (parser, CPP_OPEN_BRACE))
     add_stmt (c_parser_compound_statement (parser));
   else
-    c_parser_statement_after_labels (parser, if_p);
+    {
+      body_loc_after_labels = c_parser_peek_token (parser)->location;
+      c_parser_statement_after_labels (parser, if_p);
+    }
 
   token_indent_info next_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (if_tinfo, body_tinfo, next_tinfo);
+  if (body_loc_after_labels != UNKNOWN_LOCATION
+      && next_tinfo.type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (body_loc_after_labels, next_tinfo.location,
+				  if_tinfo.location);
 
   return c_end_compound_stmt (body_loc, block, flag_isoc99);
 }
@@ -5530,6 +5543,7 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo,
   tree block = c_begin_compound_stmt (flag_isoc99);
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
+  location_t body_loc_after_labels = UNKNOWN_LOCATION;
 
   c_parser_all_labels (parser);
   if (c_parser_next_token_is (parser, CPP_SEMICOLON))
@@ -5542,11 +5556,18 @@ c_parser_else_body (c_parser *parser, const token_indent_info &else_tinfo,
       c_parser_consume_token (parser);
     }
   else
-    c_parser_statement_after_labels (parser, NULL, chain);
+    {
+      body_loc_after_labels = c_parser_peek_token (parser)->location;
+      c_parser_statement_after_labels (parser, NULL, chain);
+    }
 
   token_indent_info next_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (else_tinfo, body_tinfo, next_tinfo);
+  if (body_loc_after_labels != UNKNOWN_LOCATION
+      && next_tinfo.type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (body_loc_after_labels, next_tinfo.location,
+				  else_tinfo.location);
 
   return c_end_compound_stmt (body_loc, block, flag_isoc99);
 }
@@ -5732,7 +5753,12 @@ c_parser_switch_statement (c_parser *parser, bool *if_p)
   c_start_case (switch_loc, switch_cond_loc, expr, explicit_cast_p);
   save_break = c_break_label;
   c_break_label = NULL_TREE;
-  body = c_parser_c99_block_statement (parser, if_p);
+  location_t loc_after_labels;
+  bool open_brace_p = c_parser_peek_token (parser)->type == CPP_OPEN_BRACE;
+  body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels);
+  location_t next_loc = c_parser_peek_token (parser)->location;
+  if (!open_brace_p && c_parser_peek_token (parser)->type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (loc_after_labels, next_loc, switch_loc);
   c_finish_case (body, ce.original_type);
   if (c_break_label)
     {
@@ -5783,7 +5809,8 @@ c_parser_while_statement (c_parser *parser, bool ivdep, bool *if_p)
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
 
-  body = c_parser_c99_block_statement (parser, if_p);
+  location_t loc_after_labels;
+  body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels);
   c_finish_loop (loc, cond, NULL, body, c_break_label, c_cont_label, true);
   add_stmt (c_end_compound_stmt (loc, block, flag_isoc99));
   c_parser_maybe_reclassify_token (parser);
@@ -5792,6 +5819,10 @@ c_parser_while_statement (c_parser *parser, bool ivdep, bool *if_p)
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (while_tinfo, body_tinfo, next_tinfo);
 
+  if (next_tinfo.type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (loc_after_labels, next_tinfo.location,
+				  while_tinfo.location);
+
   c_break_label = save_break;
   c_cont_label = save_cont;
 }
@@ -6076,7 +6107,8 @@ c_parser_for_statement (c_parser *parser, bool ivdep, bool *if_p)
   token_indent_info body_tinfo
     = get_token_indent_info (c_parser_peek_token (parser));
 
-  body = c_parser_c99_block_statement (parser, if_p);
+  location_t loc_after_labels;
+  body = c_parser_c99_block_statement (parser, if_p, &loc_after_labels);
 
   if (is_foreach_statement)
     objc_finish_foreach_loop (loc, object_expression, collection_expression, body, c_break_label, c_cont_label);
@@ -6089,6 +6121,10 @@ c_parser_for_statement (c_parser *parser, bool ivdep, bool *if_p)
     = get_token_indent_info (c_parser_peek_token (parser));
   warn_for_misleading_indentation (for_tinfo, body_tinfo, next_tinfo);
 
+  if (next_tinfo.type != CPP_SEMICOLON)
+    warn_for_multiline_expansion (loc_after_labels, next_tinfo.location,
+				  for_tinfo.location);
+
   c_break_label = save_break;
   c_cont_label = save_cont;
 }
diff --git gcc/cp/parser.c gcc/cp/parser.c
index 891341d..e2a69ce 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -2102,7 +2102,7 @@ static void cp_parser_lambda_body
 /* Statements [gram.stmt.stmt]  */
 
 static void cp_parser_statement
-  (cp_parser *, tree, bool, bool *, vec<tree> * = NULL);
+  (cp_parser *, tree, bool, bool *, vec<tree> * = NULL, location_t * = NULL);
 static void cp_parser_label_for_labeled_statement
 (cp_parser *, tree);
 static tree cp_parser_expression_statement
@@ -10531,7 +10531,8 @@ cp_parser_lambda_body (cp_parser* parser, tree lambda_expr)
 
 static void
 cp_parser_statement (cp_parser* parser, tree in_statement_expr,
-		     bool in_compound, bool *if_p, vec<tree> *chain)
+		     bool in_compound, bool *if_p, vec<tree> *chain,
+		     location_t *loc_after_labels)
 {
   tree statement, std_attrs = NULL_TREE;
   cp_token *token;
@@ -10724,6 +10725,10 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
 	  if (cp_parser_parse_definitely (parser))
 	    return;
 	}
+      /* All preceding labels have been parsed at this point.  */
+      if (loc_after_labels != NULL)
+	*loc_after_labels = statement_location;
+
       /* Look for an expression-statement instead.  */
       statement = cp_parser_expression_statement (parser, in_statement_expr);
 
@@ -12264,6 +12269,7 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
 {
   tree statement;
   location_t body_loc = cp_lexer_peek_token (parser->lexer)->location;
+  location_t body_loc_after_labels = UNKNOWN_LOCATION;
   token_indent_info body_tinfo
     = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
 
@@ -12293,7 +12299,8 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
       /* Create a compound-statement.  */
       statement = begin_compound_stmt (0);
       /* Parse the dependent-statement.  */
-      cp_parser_statement (parser, NULL_TREE, false, if_p, chain);
+      cp_parser_statement (parser, NULL_TREE, false, if_p, chain,
+			   &body_loc_after_labels);
       /* Finish the dummy compound-statement.  */
       finish_compound_stmt (statement);
     }
@@ -12302,6 +12309,11 @@ cp_parser_implicitly_scoped_statement (cp_parser* parser, bool *if_p,
     = get_token_indent_info (cp_lexer_peek_token (parser->lexer));
   warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo);
 
+  if (body_loc_after_labels != UNKNOWN_LOCATION
+      && next_tinfo.type != CPP_SEMICOLON)
+  warn_for_multiline_expansion (body_loc_after_labels, next_tinfo.location,
+				guard_tinfo.location);
+
   /* Return the statement.  */
   return statement;
 }
@@ -12320,11 +12332,17 @@ cp_parser_already_scoped_statement (cp_parser* parser, bool *if_p,
     {
       token_indent_info body_tinfo
 	= get_token_indent_info (cp_lexer_peek_token (parser->lexer));
+      location_t loc_after_labels;
 
-      cp_parser_statement (parser, NULL_TREE, false, if_p);
+      cp_parser_statement (parser, NULL_TREE, false, if_p, NULL,
+			   &loc_after_labels);
       token_indent_info next_tinfo
 	= get_token_indent_info (cp_lexer_peek_token (parser->lexer));
       warn_for_misleading_indentation (guard_tinfo, body_tinfo, next_tinfo);
+
+      if (next_tinfo.type != CPP_SEMICOLON)
+	warn_for_multiline_expansion (loc_after_labels, next_tinfo.location,
+				      guard_tinfo.location);
     }
   else
     {
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 456fa85..445fdb0 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -293,7 +293,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wmain  -Wmaybe-uninitialized  -Wmemset-elt-size  -Wmemset-transposed-args @gol
 -Wmisleading-indentation  -Wmissing-braces @gol
 -Wmissing-field-initializers  -Wmissing-include-dirs @gol
--Wno-multichar  -Wnonnull  -Wnonnull-compare @gol
+-Wno-multichar  -Wmultiline-expansion  -Wnonnull  -Wnonnull-compare @gol
 -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol
 -Wnull-dereference  -Wodr  -Wno-overflow  -Wopenmp-simd  @gol
 -Woverride-init-side-effects  -Woverlength-strings @gol
@@ -3824,6 +3824,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}.
 -Wmemset-transposed-args @gol
 -Wmisleading-indentation @r{(only for C/C++)} @gol
 -Wmissing-braces @r{(only for C/ObjC)} @gol
+-Wmultiline-expansion  @gol
 -Wnarrowing @r{(only for C++)}  @gol
 -Wnonnull  @gol
 -Wnonnull-compare  @gol
@@ -4496,6 +4497,29 @@ This warning is enabled by @option{-Wall}.
 @opindex Wno-missing-include-dirs
 Warn if a user-supplied include directory does not exist.
 
+@item -Wmultiline-expansion
+@opindex Wmultiline-expansion
+@opindex Wno-multiline-expansion
+Warn about macros expanding to multiple statements in a body of a conditional,
+such as @code{if}, @code{else}, @code{for}, or @code{while}.
+For example:
+
+@smallexample
+#define DOIT x++; y++
+if (c)
+  DOIT;
+@end smallexample
+
+will increment @code{y} unconditionally, not just when @code{c} holds.
+The can usually be fixed by wrapping the macro in a do-while loop:
+@smallexample
+#define DOIT do @{ x++; y++; @} while (0)
+if (c)
+  DOIT;
+@end smallexample
+
+This warning is enabled by @option{-Wall} in C and C++.
+
 @item -Wparentheses
 @opindex Wparentheses
 @opindex Wno-parentheses
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
index e69de29..119d119 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-1.c
@@ -0,0 +1,118 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(X, Y)	\
+  tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \
+  X = Y;		\
+  Y = tmp
+
+#define STUFF		\
+  if (0) x = y
+
+#define STUFF2		\
+  if (0) x = y; x++
+
+#define STUFF3		\
+  if (x) /* { dg-message "not guarded" } */ \
+    SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define SET(X, Y)	\
+  (X) = (Y)
+
+#define STUFF4		\
+  if (x)		\
+    SET(x, y);		\
+  SET(x, y)
+
+#define STUFF5		\
+  { tmp = x; x = y; }
+
+#define STUFF6		\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn1 (void)
+{
+  if (x) /* { dg-message "not guarded" } */
+    SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */
+}
+
+void
+fn2 (void)
+{
+  SWAP(x, y);
+}
+
+void
+fn3 (void)
+{
+  if (x)
+    {
+      SWAP(x, y);
+    }
+}
+
+void
+fn4 (void)
+{
+  if (x)
+  ({ x = 10; x++; });
+}
+
+void
+fn5 (void)
+{
+  if (x) /* { dg-message "not guarded" } */
+L1:
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+  goto L1;
+}
+
+void
+fn6 (void)
+{
+  if (x)
+    SET (x, y);
+  SET (tmp, x);
+}
+
+void
+fn7 (void)
+{
+  STUFF;
+}
+
+void
+fn8 (void)
+{
+  STUFF2;
+}
+
+void
+fn9 (void)
+{
+  STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */
+}
+
+void
+fn10 (void)
+{
+  STUFF4;
+}
+
+void
+fn11 (void)
+{
+  if (x)
+    STUFF5;
+}
+
+void
+fn12 (void)
+{
+  if (x)
+    STUFF6;
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-10.c gcc/testsuite/c-c++-common/Wmultiline-expansion-10.c
index e69de29..3e68fcb 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-10.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-10.c
@@ -0,0 +1,82 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(x, y) \
+  tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \
+  x = y; \
+  y = tmp
+
+#define M1	\
+  switch (x) /* { dg-message "not guarded" } */ \
+    case 1:	\
+    SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define M2	\
+  switch (x)	\
+    case 1:	\
+    x++
+
+#define M3	\
+  switch (x)	\
+    case 1:	\
+    x++;;
+
+#define M4	\
+  switch (x) /* { dg-message "not guarded" } */ \
+L1:		\
+    case 1:	\
+    SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define INC	\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn0 (void)
+{
+  switch (x) /* { dg-message "not guarded" } */
+    case 1:
+      SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+
+  switch (x) /* { dg-message "not guarded" } */
+    case 1:
+    case 2:
+    case 3:
+    case 4:
+    case 5:
+      SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+}
+
+void
+fn1 (void)
+{
+  M1; /* { dg-message "in expansion of macro .M1." } */
+  M2;
+  M3;
+  M4; /* { dg-message "in expansion of macro .M4." } */
+  goto L1;
+}
+
+void
+fn2 (void)
+{
+  switch (x)
+    case 1:
+      INC
+
+  switch (x)
+    case 1:
+      ({ x = 10; x++; });
+}
+
+void
+fn3 (void)
+{
+  switch (x)
+    {
+    case 1:
+      SWAP (x, y);
+    }
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
index e69de29..b39d253 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-2.c
@@ -0,0 +1,137 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(X, Y)	\
+  tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \
+  X = Y;		\
+  Y = tmp
+
+#define STUFF		\
+  if (0) {} else x = y
+
+#define STUFF2		\
+  if (0) {} else x = y; x++
+
+#define STUFF3		\
+  if (x)		\
+    {}			\
+  else /* { dg-message "not guarded" } */ \
+    SWAP(x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define SET(X, Y)	\
+  (X) = (Y)
+
+#define STUFF4		\
+  if (x)		\
+    {}			\
+  else			\
+    SET(x, y);		\
+  SET(x, y)
+
+#define STUFF5		\
+  { tmp = x; x = y; }
+
+#define STUFF6		\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn1 (void)
+{
+  if (x)
+   {
+   }
+  else /* { dg-message "not guarded" } */
+    SWAP(x, y); /* { dg-message "in expansion of macro .SWAP." } */
+}
+
+void
+fn2 (void)
+{
+  SWAP(x, y);
+}
+
+void
+fn3 (void)
+{
+  if (x)
+    {
+    }
+  else
+    {
+      SWAP(x, y);
+    }
+}
+
+void
+fn4 (void)
+{
+  if (x)
+    {
+    }
+  else
+    ({ x = 10; x++; });
+}
+
+void
+fn5 (void)
+{
+  if (x)
+    {
+    }
+  else /* { dg-message "not guarded" } */
+L1:
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+  goto L1;
+}
+
+void
+fn6 (void)
+{
+  if (x)
+    {
+    }
+  else
+    SET (x, y);
+  SET (tmp, x);
+}
+
+void
+fn7 (void)
+{
+  STUFF;
+}
+
+void
+fn8 (void)
+{
+  STUFF2;
+}
+
+void
+fn9 (void)
+{
+  STUFF3; /* { dg-message "in expansion of macro .STUFF3." } */
+}
+
+void
+fn10 (void)
+{
+  STUFF4;
+}
+
+void
+fn11 (void)
+{
+  if (x)
+    STUFF5;
+}
+
+void
+fn12 (void)
+{
+  if (x)
+    STUFF6;
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
index e69de29..c2e83af 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-3.c
@@ -0,0 +1,12 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define CHECK(X) if (!(X)) __builtin_abort ()
+
+void
+fn (int i)
+{
+  CHECK (i == 1);
+  CHECK (i == 2);
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
index e69de29..3ab76a7 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-4.c
@@ -0,0 +1,14 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define FN(C)		\
+  void			\
+  fn (void)		\
+  {			\
+    C;			\
+  }
+
+int i;
+
+FN (if (i) ++i)
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
index e69de29..01ba0de 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-5.c
@@ -0,0 +1,18 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define M(N)	\
+L ## N:		\
+  x++; x++ /* { dg-warning "macro expands to multiple statements" } */
+
+int x, y, tmp;
+
+void
+fn1 (void)
+{
+  if (x) /* { dg-message "not guarded" } */
+   M (0); /* { dg-message "in expansion of macro .M." } */
+  if (x) /* { dg-message "not guarded" } */
+   M (1); /* { dg-message "in expansion of macro .M." } */
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
index e69de29..9f799e1 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-6.c
@@ -0,0 +1,22 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define M \
+  if (x) x++; x++
+
+void
+f (int x)
+{
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+  M;
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-7.c gcc/testsuite/c-c++-common/Wmultiline-expansion-7.c
index e69de29..b561d43 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-7.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-7.c
@@ -0,0 +1,18 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(X, Y)      \
+  tmp = X; /* { dg-warning "macro expands to multiple statements" } */ \
+  X = Y;                \
+  Y = tmp
+
+#define BODY_AND_IF(COND, X, Y)  \
+  if (COND) SWAP (X, Y) /* { dg-message "in expansion of macro .SWAP." } */
+
+void
+fn (int x, int y)
+{
+  int tmp;
+  BODY_AND_IF (1, x, y); /* { dg-message "in expansion of macro .BODY_AND_IF." } */
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-8.c gcc/testsuite/c-c++-common/Wmultiline-expansion-8.c
index e69de29..c5d8205 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-8.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-8.c
@@ -0,0 +1,64 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(x, y) \
+  tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \
+  x = y; \
+  y = tmp
+
+#define M1				\
+  for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */ \
+    SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define M2				\
+  for (i = 0; i < 1; ++i)		\
+    x++
+
+#define M3				\
+  for (i = 0; i < 1; ++i)		\
+    x++;;
+
+#define M4				\
+  for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */ \
+L1:					\
+    SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define INC	\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn0 (void)
+{
+  int i;
+  for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+
+  for (i = 0; i < 1; ++i) /* { dg-message "not guarded" } */
+L:
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+  goto L;
+}
+
+void
+fn1 (void)
+{
+  int i;
+  M1; /* { dg-message "in expansion of macro .M1." } */
+  M2;
+  M3;
+  M4; /* { dg-message "in expansion of macro .M4." } */
+  goto L1;
+}
+
+void
+fn2 (void)
+{
+  for (int i = 0; i < 1; ++i)
+    INC
+
+  for (int i = 0; i < 1; ++i)
+    ({ x = 10; x++; });
+}
diff --git gcc/testsuite/c-c++-common/Wmultiline-expansion-9.c gcc/testsuite/c-c++-common/Wmultiline-expansion-9.c
index e69de29..c503ade 100644
--- gcc/testsuite/c-c++-common/Wmultiline-expansion-9.c
+++ gcc/testsuite/c-c++-common/Wmultiline-expansion-9.c
@@ -0,0 +1,62 @@
+/* PR c/80116 */
+/* { dg-options "-Wmultiline-expansion" } */
+/* { dg-do compile } */
+
+#define SWAP(x, y) \
+  tmp = x; /* { dg-warning "macro expands to multiple statements" } */ \
+  x = y; \
+  y = tmp
+
+#define M1	\
+  while (x) /* { dg-message "not guarded" } */ \
+    SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define M2	\
+  while (x)	\
+    x++
+
+#define M3	\
+  while (x)	\
+    x++;;
+
+#define M4	\
+  while (x) /* { dg-message "not guarded" } */ \
+L1:		\
+    SWAP (x, y) /* { dg-message "in expansion of macro .SWAP." } */
+
+#define INC	\
+  x++;;
+
+int x, y, tmp;
+
+void
+fn0 (void)
+{
+  while (x) /* { dg-message "not guarded" } */
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+
+  while (x) /* { dg-message "not guarded" } */
+L:
+    SWAP (x, y); /* { dg-message "in expansion of macro .SWAP." } */
+  goto L;
+}
+
+void
+fn1 (void)
+{
+  M1; /* { dg-message "in expansion of macro .M1." } */
+  M2;
+  M3;
+  M4; /* { dg-message "in expansion of macro .M4." } */
+  goto L1;
+}
+
+void
+fn2 (void)
+{
+  while (x)
+    INC
+
+  while (x)
+    ({ x = 10; x++; });
+}

	Marek

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Pedro Alves]

Hi Marek,

Nice warning!  Just to confirm, would the patch warn with code like:

 const char *
 target_xfer_status_to_string (enum target_xfer_status status)
 {
#define CASE(X) case X: return #X
   switch (status)
     {
       CASE(TARGET_XFER_E_IO);
       CASE(TARGET_XFER_UNAVAILABLE);
     default:
       return "<unknown>";
     }
#undef CASE
 };

?

I think it shouldn't, but I couldn't tell from the tests,
and the only similar instance I found in gcc is guarded
behind an #ifdef (in eh_data_format_name).

Thanks,
Pedro Alves

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Marek Polacek]

On Wed, Jun 07, 2017 at 08:02:42PM +0100, Pedro Alves wrote:
> Hi Marek,
> 
> Nice warning!  Just to confirm, would the patch warn with code like:
 
Thanks.  BTW, if you (or anyone) could come up with a better name,
I'm all ears.

>  const char *
>  target_xfer_status_to_string (enum target_xfer_status status)
>  {
> #define CASE(X) case X: return #X
>    switch (status)
>      {
>        CASE(TARGET_XFER_E_IO);
>        CASE(TARGET_XFER_UNAVAILABLE);
>      default:
>        return "<unknown>";
>      }
> #undef CASE
>  };
> 
> ?
> 
> I think it shouldn't, but I couldn't tell from the tests,

Nope, it doesn't warn (neither C nor C++).  I should probably add this test.

	Marek

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Pedro Alves]

On 06/08/2017 11:29 AM, Marek Polacek wrote:
> On Wed, Jun 07, 2017 at 08:02:42PM +0100, Pedro Alves wrote:
>> Hi Marek,
>>
>> Nice warning!  Just to confirm, would the patch warn with code like:
>  
> Thanks.  BTW, if you (or anyone) could come up with a better name,
> I'm all ears.

AFAICS, the warning's intent is catching the case of a
a macro expanding to multiple (top level) statements, not lines.
Both the comments in the code and the description of the
warning talk in those terms:

 +/* (....) This warning warns about
 +   cases when a macro expands to multiple statements not wrapped in
 +   do {} while (0) or ({ }) and is used as a body of if/else/for/while
 +   conditionals.  For example,

 +Wmultiline-expansion
 +C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
 +Warn about macros expanding to multiple statements in a body of a conditional such as if, else, while, or for.

So it'd seem clearer to me if the warning was named around
"-Wmulti-statement-something" instead of "-Wmultiline-something"?

  -Wmulti-statement-expansion
  -Wmulti-statement-macros     
  -Wmulti-statement-macro     
  -Wmulti-statement-macro-expansion

Particularly when one could argue that "multiline expansion" in
context of macros doesn't make any sense, given macros always
expand to a single line:

 #define SAME_LINE				\
 	(__LINE__				\
 	 == __LINE__)

 static_assert (SAME_LINE, "");


> Nope, it doesn't warn (neither C nor C++).  I should probably add this test.

Thanks for confirming.  A test would be nice, to make sure we 
don't regress.

Thanks,
Pedro Alves

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Marek Polacek]

On Thu, Jun 08, 2017 at 12:01:03PM +0100, Pedro Alves wrote:
> On 06/08/2017 11:29 AM, Marek Polacek wrote:
> > On Wed, Jun 07, 2017 at 08:02:42PM +0100, Pedro Alves wrote:
> >> Hi Marek,
> >>
> >> Nice warning!  Just to confirm, would the patch warn with code like:
> >  
> > Thanks.  BTW, if you (or anyone) could come up with a better name,
> > I'm all ears.
> 
> AFAICS, the warning's intent is catching the case of a
> a macro expanding to multiple (top level) statements, not lines.

True.  I felt that it was implicitly understood what's meant by that,
but I'll change that.  Martin pointed this out, too.

> Both the comments in the code and the description of the
> warning talk in those terms:
> 
>  +/* (....) This warning warns about
>  +   cases when a macro expands to multiple statements not wrapped in
>  +   do {} while (0) or ({ }) and is used as a body of if/else/for/while
>  +   conditionals.  For example,
> 
>  +Wmultiline-expansion
>  +C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
>  +Warn about macros expanding to multiple statements in a body of a conditional such as if, else, while, or for.
> 
> So it'd seem clearer to me if the warning was named around
> "-Wmulti-statement-something" instead of "-Wmultiline-something"?
> 
>   -Wmulti-statement-expansion
>   -Wmulti-statement-macros     
>   -Wmulti-statement-macro     
>   -Wmulti-statement-macro-expansion

I think I'll go with -Wmultistatement-expansion (without the dash).

> Particularly when one could argue that "multiline expansion" in
> context of macros doesn't make any sense, given macros always
> expand to a single line:
> 
>  #define SAME_LINE				\
>  	(__LINE__				\
>  	 == __LINE__)
> 
>  static_assert (SAME_LINE, "");

Sure.

> > Nope, it doesn't warn (neither C nor C++).  I should probably add this test.
> 
> Thanks for confirming.  A test would be nice, to make sure we 
> don't regress.

I'll post a new version with the warning renamed and the new test added.

Thanks,

	Marek

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Pedro Alves]

On 06/08/2017 12:19 PM, Marek Polacek wrote:
> On Thu, Jun 08, 2017 at 12:01:03PM +0100, Pedro Alves wrote:
>> On 06/08/2017 11:29 AM, Marek Polacek wrote:
>>> On Wed, Jun 07, 2017 at 08:02:42PM +0100, Pedro Alves wrote:
>>>> Hi Marek,
>>>>
>>>> Nice warning!  Just to confirm, would the patch warn with code like:
>>>  
>>> Thanks.  BTW, if you (or anyone) could come up with a better name,
>>> I'm all ears.
>>
>> AFAICS, the warning's intent is catching the case of a
>> a macro expanding to multiple (top level) statements, not lines.
> 
> True.  I felt that it was implicitly understood what's meant by that,
> but I'll change that.  Martin pointed this out, too.

I don't think it's implicit, because it could raise questions, like:

 >>  +Wmultiline-expansion
 >>  +C ObjC C++ ObjC++ Var(warn_multiline_expansion) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
 >>  +Warn about macros expanding to multiple statements in a body of a conditional such as if, else, while, or for.

"
Hmm, the description doesn't actually describe what is
meant by "multiple lines".  Does "multiline" here mean that
the warning triggers with:

 #define FOO()  \
    foo1();      \
    foo2()

but not

 #define FOO() foo1(); foo2()

?
"

It's perhaps obvious to seasoned hackers that that's not the
intention, but not all users are experts.

So a general rule I try to follow (in GDB) is: make sure that the
description of an option matches the option's name.
I.e., if the words used to name the option name don't appear in the
option's description, then that's a good indication something
is off and is bound to confuse someone.

>>
>> So it'd seem clearer to me if the warning was named around
>> "-Wmulti-statement-something" instead of "-Wmultiline-something"?
>>
>>   -Wmulti-statement-expansion
>>   -Wmulti-statement-macros     
>>   -Wmulti-statement-macro     
>>   -Wmulti-statement-macro-expansion
> 
> I think I'll go with -Wmultistatement-expansion (without the dash).

Fine with me, FWIW.

> I'll post a new version with the warning renamed and the new test added.

Great, thanks much!

Thanks,
Pedro Alves

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Martin Sebor]

>> I think I'll go with -Wmultistatement-expansion (without the dash).
>
> Fine with me, FWIW.

I like this name better, but I think -Wmultistatement-macros
would be clearer (it also matches the CERT rule).

Martin

Re: C/C++ PATCH to implement -Wmultiline-expansion (PR c/80116)

[Marek Polacek]

On Thu, Jun 08, 2017 at 08:53:54AM -0600, Martin Sebor wrote:
> > > I think I'll go with -Wmultistatement-expansion (without the dash).
> > 
> > Fine with me, FWIW.
> 
> I like this name better, but I think -Wmultistatement-macros
> would be clearer (it also matches the CERT rule).

Ok, I'll buy this.  I'll rename the warning once again :).

	Marek