diff mbox

c-family PATCH to improve and simplify -Wmultistatement-macros (PR c/81448, c/81306)

Message ID 20170727144420.GK3397@redhat.com
State New
Headers show

Commit Message

Marek Polacek July 27, 2017, 2:44 p.m. UTC 
To recap, -Wmultistatement-macros warns for code like

#define FOO y++; z++

  if (i)
     FOO;

if FOO expands to multiple statements not wrapped in {}.  It tracks the
location of the guard (if), the location of the body of the conditional
(y++) and the location of the following statement (z++).  This warning
only warns if BODY and NEXT come from the same macro expansion, and the
guard doesn't come from that expansion.  But it can be fooled with code
like

#define IF if(1)

#define BAR				\
void bar (void)				\
{					\
  IF					\
    baz ();				\
  return;				\
}

where BODY and NEXT come from the same expansion (BAR), the guard doesn't, yet
we shouldn't warn.  To stave off the bogus warnings, my fix is to keep
unwinding the IF macro, and if any expansion is the same as the expansion of
BODY and LOC, don't warn.  So basically avoid the warning in this scenario:

               BAR
                |
      +------------------+
      |                  |
      IF            baz (); return;
      |
      if

While working on the fix, I noticed I can simplify the code a lot, the
MACRO_MAP_LOCATIONS walk is not necessary -- which should also fix the
ugly PR81306 - yay!

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

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

	PR c/81448
	PR c/81306
	* c-warn.c (warn_for_multistatement_macros): Prevent bogus
	warnings.  Avoid walking MACRO_MAP_LOCATIONS.						     

	* c-c++-common/Wmultistatement-macros-13.c: New test.


	Marek

Comments

Joseph Myers Aug. 2, 2017, 11:37 a.m. UTC | #1 
On Thu, 27 Jul 2017, Marek Polacek wrote:

> Bootstrapped/regtested on x86_64-linux and ppc64le-linux, ok for trunk?
> 
> 2017-07-27  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c/81448
> 	PR c/81306
> 	* c-warn.c (warn_for_multistatement_macros): Prevent bogus
> 	warnings.  Avoid walking MACRO_MAP_LOCATIONS.						     
> 
> 	* c-c++-common/Wmultistatement-macros-13.c: New test.

OK.  Steve, as I noted in 
<https://sourceware.org/ml/libc-alpha/2017-07/msg00942.html>, please see 
if this means you no longer need to use -Wno-multistatement-macros for 
certain glibc tests (if it's still needed, that suggests another related 
problem may still be present).
Marek Polacek Aug. 2, 2017, 12:12 p.m. UTC | #2 
On Wed, Aug 02, 2017 at 11:37:27AM +0000, Joseph Myers wrote:
> On Thu, 27 Jul 2017, Marek Polacek wrote:
> 
> > Bootstrapped/regtested on x86_64-linux and ppc64le-linux, ok for trunk?
> > 
> > 2017-07-27  Marek Polacek  <polacek@redhat.com>
> > 
> > 	PR c/81448
> > 	PR c/81306
> > 	* c-warn.c (warn_for_multistatement_macros): Prevent bogus
> > 	warnings.  Avoid walking MACRO_MAP_LOCATIONS.						     
> > 
> > 	* c-c++-common/Wmultistatement-macros-13.c: New test.
> 
> OK.  Steve, as I noted in 
> <https://sourceware.org/ml/libc-alpha/2017-07/msg00942.html>, please see 
> if this means you no longer need to use -Wno-multistatement-macros for 
> certain glibc tests (if it's still needed, that suggests another related 
> problem may still be present).

I've committed the patch (r250822), so I hope all those bogus warnings will
disappear.

	Marek
diff mbox

Patch

diff --git gcc/c-family/c-warn.c gcc/c-family/c-warn.c
index a8b38c1b98d..0bfb24994d9 100644
--- gcc/c-family/c-warn.c
+++ gcc/c-family/c-warn.c
@@ -2456,34 +2456,44 @@  warn_for_multistatement_macros (location_t body_loc, location_t next_loc,
       || 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;
-    }
+  /* Find the macro maps for the macro expansions.  */
+  const line_map *body_map = linemap_lookup (line_table, body_loc);
+  const line_map *next_map = linemap_lookup (line_table, next_loc);
+  const line_map *guard_map = linemap_lookup (line_table, guard_loc);
+
+  /* Now see if the following token (after the body) is coming from the
+     same macro expansion.  If it is, it might be a problem.  */
+  if (body_map != next_map)
+    return;
 
   /* 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)
+  if (guard_map == body_map)
     return;
 
+  /* Handle the case where NEXT and BODY come from the same expansion while
+     GUARD doesn't, yet we shouldn't warn.  E.g.
+
+       #define GUARD if (...)
+       #define GUARD2 GUARD
+
+     and in the definition of another macro:
+
+       GUARD2
+	foo ();
+       return 1;
+   */
+  while (linemap_macro_expansion_map_p (guard_map))
+    {
+      const line_map_macro *mm = linemap_check_macro (guard_map);
+      guard_loc_exp = MACRO_MAP_EXPANSION_POINT_LOCATION (mm);
+      guard_map = linemap_lookup (line_table, guard_loc_exp);
+      if (guard_map == body_map)
+	return;
+    }
+
   if (warning_at (body_loc, OPT_Wmultistatement_macros,
 		  "macro expands to multiple statements"))
     inform (guard_loc, "some parts of macro expansion are not guarded by "
diff --git gcc/testsuite/c-c++-common/Wmultistatement-macros-13.c gcc/testsuite/c-c++-common/Wmultistatement-macros-13.c
index e69de29bb2d..9f42e268d9f 100644
--- gcc/testsuite/c-c++-common/Wmultistatement-macros-13.c
+++ gcc/testsuite/c-c++-common/Wmultistatement-macros-13.c
@@ -0,0 +1,104 @@ 
+/* PR c/81448 */
+/* { dg-do compile } */
+/* { dg-options "-Wmultistatement-macros" } */
+
+extern int i;
+
+#define BAD4 i++; i++ /* { dg-warning "macro expands to multiple statements" } */
+#define BAD5 i++; i++ /* { dg-warning "macro expands to multiple statements" } */
+#define BAD6 i++; i++ /* { dg-warning "macro expands to multiple statements" } */
+#define BAD7 i++; i++ /* { dg-warning "macro expands to multiple statements" } */
+#define BAD8 i++; i++ /* { dg-warning "macro expands to multiple statements" } */
+#define BAD9 i++; i++ /* { dg-warning "macro expands to multiple statements" } */
+#define IF if (1) /* { dg-message "not guarded by this 'if' clause" } */
+#define IF2 IF /* { dg-message "in expansion of macro .IF." } */
+#define BADB7 BAD7 /* { dg-message "in expansion of macro .BAD7." } */
+#define BADB8 BAD8 /* { dg-message "in expansion of macro .BAD8." } */
+#define BADB9 BAD9 /* { dg-message "in expansion of macro .BAD9." } */
+
+#define FN0				\
+void fn0 (void)				\
+{					\
+  IF					\
+    i++;				\
+  return;				\
+}
+
+#define FN1				\
+void fn1 (void)				\
+{					\
+  IF2					\
+    i++;				\
+  return;				\
+}
+
+#define FN2				\
+void fn2 (void)				\
+{					\
+  if (1)				\
+    i++;				\
+  return;				\
+}
+
+#define TOP FN3
+#define FN3				\
+void fn3 (void)				\
+{					\
+  IF					\
+    i++;				\
+  return;				\
+}
+
+#define TOP2 FN4 /* { dg-message "in expansion of macro .FN4." } */
+#define FN4				\
+void fn4 (void)				\
+{					\
+  IF2 /* { dg-message "in expansion of macro .IF2." } */ \
+    BAD4; /* { dg-message "in expansion of macro .BAD4." } */ \
+}
+
+#define FN5				\
+void fn5 (void)				\
+{					\
+  IF /* { dg-message "in expansion of macro .IF." } */	\
+    BAD5; /* { dg-message "in expansion of macro .BAD5." } */ \
+}
+
+#define FN6				\
+void fn6 (void)				\
+{					\
+  if (1) /* { dg-message "not guarded by this 'if' clause" } */ \
+    BAD6; /* { dg-message "in expansion of macro .BAD6." } */ \
+}
+
+#define FN7				\
+void fn7 (void)				\
+{					\
+  if (1) /* { dg-message "not guarded by this 'if' clause" } */	\
+    BADB7; /* { dg-message "in expansion of macro .BADB7." } */ \
+}
+
+#define FN8				\
+void fn8 (void)				\
+{					\
+  IF2 /* { dg-message "in expansion of macro .IF2." } */ \
+    BADB8; /* { dg-message "in expansion of macro .BADB8." } */ \
+}
+
+#define FN9				\
+void fn9 (void)				\
+{					\
+  IF /* { dg-message "in expansion of macro .IF." } */ \
+    BADB9; /* { dg-message "in expansion of macro .BADB9." } */	\
+}
+
+FN0
+FN1
+FN2
+TOP
+TOP2 /* { dg-message "in expansion of macro .TOP2." } */
+FN5 /* { dg-message "in expansion of macro .FN5." } */
+FN6 /* { dg-message "in expansion of macro .FN6." } */
+FN7 /* { dg-message "in expansion of macro .FN7." } */
+FN8 /* { dg-message "in expansion of macro .FN8." } */
+FN9 /* { dg-message "in expansion of macro .FN9." } */