public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076]
@ 2020-07-28 19:50 Patrick Palka
  2020-07-28 20:52 ` David Malcolm
  2020-08-10 13:46 ` Patrick Palka
  0 siblings, 2 replies; 8+ messages in thread
From: Patrick Palka @ 2020-07-28 19:50 UTC (permalink / raw)
  To: gcc-patches

Currently the -Wmisleading-indentation warning doesn't do any analysis
when the guarded statement or the statement after it is produced by a
macro, as the mentioned PR illustrates.  This means that we warn for:

  if (flag)
    foo ();
    bar ();

and yet we don't warn for:

  #define BAR bar
  if (flag)
    foo ();
    BAR ();

which seems like a surprising limitation.

This patch extends the -Wmisleading-indentation implementation to
support analyzing such statements and their tokens.  This is done in the
"natural" way by resolving the location of each of the three tokens to
the token's macro expansion point.  (Additionally, if the tokens all
resolve to the same macro expansion point then we instead use their
locations within the macro definition.)  When these resolved locations
are all different, then we can proceed with applying the warning
heuristics to them as if no macros were involved.

Bootstrapped and regtested on x86_64-pc-linux-gnu,
powerpc64le-unknown-linux-gnu, aarch64-unknown-linux-gnu and
s390x-ibm-linux-gnu.

I also built sqlite3, glibc, linux, and a number of other projects with
this patch, to help verify that it doesn't introduce bogus warnings.
All new warnings that I've seen look justified from what I can tell,
e.g. we now warn about the following line, which is indented as if it's
guarded by the if statement two lines above it:
https://github.com/torvalds/linux/blob/fe557319aa06c23cffc9346000f119547e0f289a/drivers/misc/kgdbts.c#L105

Does this look OK to commit?

gcc/c-family/ChangeLog:

	PR c/80076
	* c-indentation.c (should_warn_for_misleading_indentation): Move
	declarations of local variables closer to their first use.
	Handle virtual token locations by resolving them to their
	respective macro expansion points.  If all three tokens are
	produced from the same macro expansion, then instead use their
	loci within the macro definition.

gcc/objc/ChangeLog:

	PR c/80076
	* objc-gnu-runtime-abi-01.c
	(gnu_runtime_abi_01_get_class_super_ref): Reduce indentation of
	misleadingly indented return statements.
	* objc-next-runtime-abi-01.c
	(next_runtime_abi_01_get_class_super_ref): Likewise.

gcc/ChangeLog:

	PR c/80076
	* gensupport.c (alter_attrs_for_subst_insn) <case SET_ATTR>:
	Reduce indentation of misleadingly indented code fragment.
	* lra-constraints.c (multi_block_pseudo_p): Likewise.
	* sel-sched-ir.c (merge_fences): Likewise.

libcpp/ChangeLog:

	PR c/80076
	* include/line-map.h (first_map_in_common): Declare.
	* line-map.c (first_map_in_common): Remove static.

gcc/testsuite/ChangeLog:

	PR c/80076
	* c-c++-common/Wmisleading-indentation-pr80076.c: New test.
---
 gcc/c-family/c-indentation.c                  |  61 ++++++++--
 gcc/gensupport.c                              |   2 +-
 gcc/lra-constraints.c                         |  12 +-
 gcc/objc/objc-gnu-runtime-abi-01.c            |   4 +-
 gcc/objc/objc-next-runtime-abi-01.c           |   4 +-
 gcc/sel-sched-ir.c                            | 112 +++++++++---------
 .../c-c++-common/Wmisleading-indentation-5.c  |  56 +++++++++
 libcpp/include/line-map.h                     |   6 +
 libcpp/line-map.c                             |   2 +-
 9 files changed, 178 insertions(+), 81 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c

diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index d814f6f29e6..8b88a8adc7c 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -213,19 +213,6 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 					const token_indent_info &body_tinfo,
 					const token_indent_info &next_tinfo)
 {
-  location_t guard_loc = guard_tinfo.location;
-  location_t body_loc = body_tinfo.location;
-  location_t next_stmt_loc = next_tinfo.location;
-
-  enum cpp_ttype body_type = body_tinfo.type;
-  enum cpp_ttype next_tok_type = next_tinfo.type;
-
-  /* Don't attempt to compare the indentation of BODY_LOC and NEXT_STMT_LOC
-     if either are within macros.  */
-  if (linemap_location_from_macro_expansion_p (line_table, body_loc)
-      || linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
-    return false;
-
   /* Don't attempt to compare indentation if #line or # 44 "file"-style
      directives are present, suggesting generated code.
 
@@ -266,6 +253,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
      }            <- NEXT
      baz ();
   */
+  enum cpp_ttype next_tok_type = next_tinfo.type;
   if (next_tok_type == CPP_CLOSE_BRACE
       || next_tinfo.keyword == RID_ELSE)
     return false;
@@ -287,6 +275,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
       bar (); <- BODY
       baz (); <- NEXT
   */
+  enum cpp_ttype body_type = body_tinfo.type;
   if (body_type == CPP_OPEN_BRACE)
     return false;
 
@@ -294,6 +283,52 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
   if (next_tok_type == CPP_SEMICOLON)
     return false;
 
+  location_t guard_loc = guard_tinfo.location;
+  location_t body_loc = body_tinfo.location;
+  location_t next_stmt_loc = next_tinfo.location;
+
+  /* Resolve each token location to the respective macro expansion
+     point that produced the token.  */
+  if (linemap_location_from_macro_expansion_p (line_table, guard_loc))
+    guard_loc = linemap_resolve_location (line_table, guard_loc,
+					  LRK_MACRO_EXPANSION_POINT, NULL);
+  if (linemap_location_from_macro_expansion_p (line_table, body_loc))
+    body_loc = linemap_resolve_location (line_table, body_loc,
+					 LRK_MACRO_EXPANSION_POINT, NULL);
+  if (linemap_location_from_macro_expansion_p (line_table, next_stmt_loc))
+    next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
+					      LRK_MACRO_EXPANSION_POINT, NULL);
+
+  /* When all three tokens are produced from a single macro expansion, we
+     instead consider their loci inside that macro's definition.  */
+  if (guard_loc == body_loc && body_loc == next_stmt_loc)
+    {
+      const line_map *guard_body_common_map
+	= first_map_in_common (line_table,
+			       guard_tinfo.location, body_tinfo.location,
+			       &guard_loc, &body_loc);
+      const line_map *body_next_common_map
+	= first_map_in_common (line_table,
+			       body_tinfo.location, next_tinfo.location,
+			       &body_loc, &next_stmt_loc);
+
+      /* Punt on complicated nesting of macros.  */
+      if (guard_body_common_map != body_next_common_map)
+	return false;
+
+      guard_loc = linemap_resolve_location (line_table, guard_loc,
+					    LRK_MACRO_DEFINITION_LOCATION, NULL);
+      body_loc = linemap_resolve_location (line_table, body_loc,
+					   LRK_MACRO_DEFINITION_LOCATION, NULL);
+      next_stmt_loc = linemap_resolve_location (line_table, next_stmt_loc,
+						LRK_MACRO_DEFINITION_LOCATION,
+						NULL);
+    }
+
+  /* Give up if the loci are not all distinct.  */
+  if (guard_loc == body_loc || body_loc == next_stmt_loc)
+    return false;
+
   expanded_location body_exploc = expand_location (body_loc);
   expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
   expanded_location guard_exploc = expand_location (guard_loc);
diff --git a/gcc/gensupport.c b/gcc/gensupport.c
index f2ad54f0c55..61691aadff1 100644
--- a/gcc/gensupport.c
+++ b/gcc/gensupport.c
@@ -1501,7 +1501,7 @@ alter_attrs_for_subst_insn (class queue_elem * elem, int n_dup)
 	case SET_ATTR:
 	  if (strchr (XSTR (sub, 1), ',') != NULL)
 	    XSTR (sub, 1) = duplicate_alternatives (XSTR (sub, 1), n_dup);
-	    break;
+	  break;
 
 	case SET_ATTR_ALTERNATIVE:
 	case SET:
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 421c453997b..90ca31632f1 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -4707,12 +4707,12 @@ multi_block_pseudo_p (int regno)
   if (regno < FIRST_PSEUDO_REGISTER)
     return false;
 
-    EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
-      if (bb == NULL)
-	bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
-      else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
-	return true;
-    return false;
+  EXECUTE_IF_SET_IN_BITMAP (&lra_reg_info[regno].insn_bitmap, 0, uid, bi)
+    if (bb == NULL)
+      bb = BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn);
+    else if (BLOCK_FOR_INSN (lra_insn_recog_data[uid]->insn) != bb)
+      return true;
+  return false;
 }
 
 /* Return true if LIST contains a deleted insn.  */
diff --git a/gcc/objc/objc-gnu-runtime-abi-01.c b/gcc/objc/objc-gnu-runtime-abi-01.c
index d5862435c29..c9959a7e1e8 100644
--- a/gcc/objc/objc-gnu-runtime-abi-01.c
+++ b/gcc/objc/objc-gnu-runtime-abi-01.c
@@ -821,7 +821,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
 	ucls_super_ref =
 		objc_build_component_ref (imp->class_decl,
 					  get_identifier ("super_class"));
-	return ucls_super_ref;
+      return ucls_super_ref;
     }
   else
     {
@@ -829,7 +829,7 @@ gnu_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
 	uucls_super_ref =
 		objc_build_component_ref (imp->meta_decl,
 					  get_identifier ("super_class"));
-	return uucls_super_ref;
+      return uucls_super_ref;
     }
 }
 
diff --git a/gcc/objc/objc-next-runtime-abi-01.c b/gcc/objc/objc-next-runtime-abi-01.c
index 5c34fcb05cb..233d89e75b5 100644
--- a/gcc/objc/objc-next-runtime-abi-01.c
+++ b/gcc/objc/objc-next-runtime-abi-01.c
@@ -938,7 +938,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
 	ucls_super_ref =
 		objc_build_component_ref (imp->class_decl,
 					  get_identifier ("super_class"));
-	return ucls_super_ref;
+      return ucls_super_ref;
     }
   else
     {
@@ -946,7 +946,7 @@ next_runtime_abi_01_get_class_super_ref (location_t loc ATTRIBUTE_UNUSED,
 	uucls_super_ref =
 		objc_build_component_ref (imp->meta_decl,
 					  get_identifier ("super_class"));
-	return uucls_super_ref;
+      return uucls_super_ref;
     }
 }
 
diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c
index 4f1e4afdc4c..1dd981ff8d5 100644
--- a/gcc/sel-sched-ir.c
+++ b/gcc/sel-sched-ir.c
@@ -722,63 +722,63 @@ merge_fences (fence_t f, insn_t insn,
                         != BLOCK_FOR_INSN (last_scheduled_insn));
           }
 
-        /* Find edge of first predecessor (last_scheduled_insn_old->insn).  */
-        FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
-                         SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
-          {
-            if (succ == insn)
-              {
-                /* No same successor allowed from several edges.  */
-                gcc_assert (!edge_old);
-                edge_old = si.e1;
-              }
-          }
-        /* Find edge of second predecessor (last_scheduled_insn->insn).  */
-        FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
-                         SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
-          {
-            if (succ == insn)
-              {
-                /* No same successor allowed from several edges.  */
-                gcc_assert (!edge_new);
-                edge_new = si.e1;
-              }
-          }
+      /* Find edge of first predecessor (last_scheduled_insn_old->insn).  */
+      FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn_old,
+		       SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
+	{
+	  if (succ == insn)
+	    {
+	      /* No same successor allowed from several edges.  */
+	      gcc_assert (!edge_old);
+	      edge_old = si.e1;
+	    }
+	}
+      /* Find edge of second predecessor (last_scheduled_insn->insn).  */
+      FOR_EACH_SUCC_1 (succ, si, last_scheduled_insn,
+		       SUCCS_NORMAL | SUCCS_SKIP_TO_LOOP_EXITS)
+	{
+	  if (succ == insn)
+	    {
+	      /* No same successor allowed from several edges.  */
+	      gcc_assert (!edge_new);
+	      edge_new = si.e1;
+	    }
+	}
 
-        /* Check if we can choose most probable predecessor.  */
-        if (edge_old == NULL || edge_new == NULL)
-          {
-            reset_deps_context (FENCE_DC (f));
-            delete_deps_context (dc);
-            vec_free (executing_insns);
-            free (ready_ticks);
-
-            FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
-            if (FENCE_EXECUTING_INSNS (f))
-              FENCE_EXECUTING_INSNS (f)->block_remove (0,
-                                FENCE_EXECUTING_INSNS (f)->length ());
-            if (FENCE_READY_TICKS (f))
-              memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
-          }
-        else
-          if (edge_new->probability > edge_old->probability)
-            {
-              delete_deps_context (FENCE_DC (f));
-              FENCE_DC (f) = dc;
-              vec_free (FENCE_EXECUTING_INSNS (f));
-              FENCE_EXECUTING_INSNS (f) = executing_insns;
-              free (FENCE_READY_TICKS (f));
-              FENCE_READY_TICKS (f) = ready_ticks;
-              FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
-              FENCE_CYCLE (f) = cycle;
-            }
-          else
-            {
-              /* Leave DC and CYCLE untouched.  */
-              delete_deps_context (dc);
-              vec_free (executing_insns);
-              free (ready_ticks);
-            }
+      /* Check if we can choose most probable predecessor.  */
+      if (edge_old == NULL || edge_new == NULL)
+	{
+	  reset_deps_context (FENCE_DC (f));
+	  delete_deps_context (dc);
+	  vec_free (executing_insns);
+	  free (ready_ticks);
+
+	  FENCE_CYCLE (f) = MAX (FENCE_CYCLE (f), cycle);
+	  if (FENCE_EXECUTING_INSNS (f))
+	    FENCE_EXECUTING_INSNS (f)->block_remove (0,
+			      FENCE_EXECUTING_INSNS (f)->length ());
+	  if (FENCE_READY_TICKS (f))
+	    memset (FENCE_READY_TICKS (f), 0, FENCE_READY_TICKS_SIZE (f));
+	}
+      else
+	if (edge_new->probability > edge_old->probability)
+	  {
+	    delete_deps_context (FENCE_DC (f));
+	    FENCE_DC (f) = dc;
+	    vec_free (FENCE_EXECUTING_INSNS (f));
+	    FENCE_EXECUTING_INSNS (f) = executing_insns;
+	    free (FENCE_READY_TICKS (f));
+	    FENCE_READY_TICKS (f) = ready_ticks;
+	    FENCE_READY_TICKS_SIZE (f) = ready_ticks_size;
+	    FENCE_CYCLE (f) = cycle;
+	  }
+	else
+	  {
+	    /* Leave DC and CYCLE untouched.  */
+	    delete_deps_context (dc);
+	    vec_free (executing_insns);
+	    free (ready_ticks);
+	  }
     }
 
   /* Fill remaining invariant fields.  */
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
new file mode 100644
index 00000000000..12b53569ba7
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-5.c
@@ -0,0 +1,56 @@
+/* PR c/80076  */
+/* { dg-options "-Wmisleading-indentation" }  */
+
+void foo(void);
+
+void test01(int flag) {
+#define bar() foo() /* { dg-message "this statement" }  */
+  if (flag) /* { dg-warning "does not guard" }  */
+    foo();
+    bar(); /* { dg-message "in expansion of macro" }  */
+#undef bar
+}
+
+void test02(int flag) {
+#define bar() foo()
+  if (flag) /* { dg-warning "does not guard" }  */
+    bar();
+    foo(); /* { dg-message "this statement" }  */
+#undef bar
+}
+
+void test03(int flag) {
+#define bar() foo() /* { dg-message "this statement" }  */
+  if (flag) /* { dg-warning "does not guard" }  */
+    bar();
+    bar(); /* { dg-message "in expansion of macro" }  */
+#undef bar
+}
+
+void test04(int flag, int num) {
+#define bar() \
+  {		\
+    if (flag)	\
+      num = 0;	\
+      num = 1;	\
+  }
+  bar();
+/* { dg-warning "does not guard" "" { target *-*-* } .-5 }  */
+/* { dg-message "this statement" "" { target *-*-* } .-4 }  */
+#undef bar
+}
+
+void test05(int flag, int num) {
+#define baz() (num = 1)
+#define bar() \
+  {		\
+    if (flag)	\
+      num = 0;	\
+      baz();	\
+  }
+#define wrapper bar
+  wrapper();
+/* { dg-warning "does not guard" "" { target *-*-* } .-6 }  */
+/* { dg-message "this statement" "" { target *-*-* } .-10 }  */
+#undef bar
+}
diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index 217f916fc35..44008be5c08 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -1225,6 +1225,12 @@ LINEMAP_SYSP (const line_map_ordinary *ord_map)
   return ord_map->sysp;
 }
 
+const struct line_map *first_map_in_common (line_maps *set,
+					    location_t loc0,
+					    location_t loc1,
+					    location_t *res_loc0,
+					    location_t *res_loc1);
+
 /* Return a positive value if PRE denotes the location of a token that
    comes before the token of POST, 0 if PRE denotes the location of
    the same token as the token for POST, and a negative value
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index a8d52861dee..5a74174579f 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -1289,7 +1289,7 @@ first_map_in_common_1 (line_maps *set,
    virtual location of the token inside the resulting macro, upon
    return of a non-NULL result.  */
 
-static const struct line_map*
+const struct line_map*
 first_map_in_common (line_maps *set,
 		     location_t loc0,
 		     location_t loc1,
-- 
2.28.0.rc1


^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-17 18:30 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 19:50 [PATCH] c-family: Macro support in -Wmisleading-indentation [PR80076] Patrick Palka
2020-07-28 20:52 ` David Malcolm
2020-07-29  0:22   ` Patrick Palka
2020-09-10 17:01     ` David Malcolm
2020-09-10 19:53       ` Patrick Palka
2020-09-17 18:30         ` Patrick Palka
2020-08-10 13:46 ` Patrick Palka
2020-09-10 13:40   ` Patrick Palka

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).