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 <pola...@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 <pola...@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
