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

[Martin Sebor <msebor at gmail dot com>]

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