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 <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).
> + 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
