[gcc(refs/users/aoliva/heads/testme)] hardcfr: add checking at exceptions and noreturn calls: tweaks

[Alexandre Oliva <aoliva@gcc.gnu.org>]

https://gcc.gnu.org/g:b949cab955834b30bddc12f970b44030f7c1f8a9

commit b949cab955834b30bddc12f970b44030f7c1f8a9
Author: Alexandre Oliva <oliva@gnu.org>
Date:   Wed Aug 10 22:56:28 2022 -0300

    hardcfr: add checking at exceptions and noreturn calls: tweaks

Diff:
---
 gcc/gimple-harden-control-flow.cc | 167 ++++++++++++++++++++++++--------------
 libgcc/hardcfr.c                  | 109 +++++++++++++++++++++++--
 2 files changed, 207 insertions(+), 69 deletions(-)

diff --git a/gcc/gimple-harden-control-flow.cc b/gcc/gimple-harden-control-flow.cc
index 5066a43fe6c..c673774887b 100644
--- a/gcc/gimple-harden-control-flow.cc
+++ b/gcc/gimple-harden-control-flow.cc
@@ -113,7 +113,8 @@ public:
       }
 
     if (param_hardcfr_max_blocks > 0
-	&& n_basic_blocks_for_fn (fun) - 2 > param_hardcfr_max_blocks)
+	&& (n_basic_blocks_for_fn (fun) - NUM_FIXED_BLOCKS
+	    > param_hardcfr_max_blocks))
       {
 	if (flag_harden_control_flow_redundancy < 0)
 	  return false;
@@ -173,8 +174,8 @@ class rt_bb_visited
      neither ENTRY nor EXIT, but maybe one-past-the-end, to compute
      the visited array length.  */
   blknum num2idx (blknum n) {
-    gcc_checking_assert (n >= 2 && n <= nblocks);
-    return (n - 2);
+    gcc_checking_assert (n >= NUM_FIXED_BLOCKS && n <= nblocks);
+    return (n - NUM_FIXED_BLOCKS);
   }
   /* Return the block vindex for BB, that must not be ENTRY or
      EXIT.  */
@@ -350,7 +351,7 @@ public:
 					 NULL, NULL);
     gimple_seq_add_stmt (&ckseq, detach);
 
-    if (nblocks - 2 > blknum (param_hardcfr_max_inline_blocks)
+    if (nblocks - NUM_FIXED_BLOCKS > blknum (param_hardcfr_max_inline_blocks)
 	|| !single_pred_p (EXIT_BLOCK_PTR_FOR_FN (cfun))
 	|| (EDGE_COUNT (EXIT_BLOCK_PTR_FOR_FN (cfun)->preds)
 	    + noreturn_blocks > 1))
@@ -669,7 +670,7 @@ public:
   /* Add to BB code to set its bit in VISITED, and add to RTCFG or
      CKSEQ the data or code needed to check BB's predecessors and
      successors.  Do NOT change the CFG.  */
-  void visit (basic_block bb)
+  void visit (basic_block bb, bool noreturn)
   {
     /* Set the bit in VISITED when entering the block.  */
     gimple_stmt_iterator gsi = gsi_after_labels (bb);
@@ -689,10 +690,13 @@ public:
 	rtcfg = tree_cons (NULL_TREE, build_int_cst (vword_type, 0), rtcfg);
 
 	/* Then, successors.  */
-	for (int i = EDGE_COUNT (bb->succs); i--; )
-	  if (push_rtcfg_pair (EDGE_SUCC (bb, i)->dest, bb,
-			       EXIT_BLOCK_PTR_FOR_FN (cfun)))
-	    break;
+	if (!noreturn
+	    || !push_rtcfg_pair (EXIT_BLOCK_PTR_FOR_FN (cfun),
+				 bb, EXIT_BLOCK_PTR_FOR_FN (cfun)))
+	  for (int i = EDGE_COUNT (bb->succs); i--; )
+	    if (push_rtcfg_pair (EDGE_SUCC (bb, i)->dest, bb,
+				 EXIT_BLOCK_PTR_FOR_FN (cfun)))
+	      break;
 	rtcfg = tree_cons (NULL_TREE, build_int_cst (vword_type, 0), rtcfg);
       }
     else
@@ -713,6 +717,8 @@ public:
 	gassign *blkruns = gimple_build_assign (ckpart, unshare_expr (bit));
 	gimple_seq_add_stmt (&ckseq, blkruns);
 
+	if (noreturn)
+	  build_block_check (EXIT_BLOCK_PTR_FOR_FN (cfun));
 	for (int i = 0, e = EDGE_COUNT (bb->succs); i < e; i++)
 	  build_block_check (EDGE_SUCC (bb, i)->dest);
 
@@ -729,10 +735,12 @@ public:
 unsigned int
 pass_harden_control_flow_redundancy::execute (function *fun)
 {
+  bool param_check_at_escaping_exceptions = true;
+  bool param_check_before_noreturn_calls = true;
   basic_block bb_eh_cleanup = NULL;
   basic_block bb;
 
-  if (flag_exceptions)
+  if (param_check_at_escaping_exceptions && flag_exceptions)
     {
       int lp_eh_cleanup = -1;
 
@@ -768,6 +776,11 @@ pass_harden_control_flow_redundancy::execute (function *fun)
 
 	      if (!stmt_ends_bb_p (stmt))
 		split_block (bb, stmt);
+	      /* A noreturn call needs not be associated with the
+		 cleanup handler, because we'll add checking before
+		 the call.  */
+	      else if (EDGE_COUNT (bb->succs) == 0)
+		continue;
 
 	      if (!bb_eh_cleanup)
 		{
@@ -791,15 +804,32 @@ pass_harden_control_flow_redundancy::execute (function *fun)
 		  gresx *resx = gimple_build_resx (new_r->index);
 		  gsi_insert_before (&ehgsi, resx, GSI_SAME_STMT);
 		}
-	      else
+	      else if (dom_info_available_p (CDI_DOMINATORS))
 		{
-		  // Update immedite dominator and loop?
+		  basic_block immdom;
+		  immdom = get_immediate_dominator (CDI_DOMINATORS, 
+						    bb_eh_cleanup);
+		  if (!dominated_by_p (CDI_DOMINATORS, immdom, bb))
+		    {
+		      immdom = nearest_common_dominator (CDI_DOMINATORS,
+							 immdom, bb);
+		      set_immediate_dominator (CDI_DOMINATORS,
+					       bb_eh_cleanup, immdom);
+		    }
 		}
 
 	      add_stmt_to_eh_lp (stmt, lp_eh_cleanup);
 	      /* Finally, wire the EH cleanup block into the CFG.  */
-	      make_eh_edges (stmt);		}
+	      make_eh_edges (stmt);
+	    }
 	}
+
+#if 1
+  /* Renumbering blocks makes for a more compact rtcfg
+     representation.  */
+      if (bb_eh_cleanup)
+	cleanup_tree_cfg ();
+#endif
     }
 
   /* We wish to add verification at blocks without successors, such as
@@ -809,57 +839,61 @@ pass_harden_control_flow_redundancy::execute (function *fun)
   int count_noreturn = 0;
   auto_sbitmap noreturn_blocks (last_basic_block_for_fn (fun));
   bitmap_clear (noreturn_blocks);
-  FOR_EACH_BB_FN (bb, fun)
-    {
-      if (EDGE_COUNT (bb->succs) == 0)
-	{
-	  if (bitmap_set_bit (noreturn_blocks, bb->index))
-	    count_noreturn++;
+  if (!param_check_before_noreturn_calls)
+    FOR_EACH_BB_FN (bb, fun)
+      {
+	if (EDGE_COUNT (bb->succs) == 0)
+	  {
+	    if (bitmap_set_bit (noreturn_blocks, bb->index))
+	      count_noreturn++;
+	    continue;
+	  }
+
+	/* If there are no exceptions, then any noreturn call must have
+	   zero successor edges.  Otherwise, check for blocks without
+	   non-EH successors, but skip those with resx stmts and edges
+	   (i.e., those other than that in bb_eh_cleanup), since those
+	   will go through bb_eh_cleanup, that will have been counted as
+	   noreturn above because it has no successors.  */
+	gcc_checking_assert (bb != bb_eh_cleanup);
+	if (!flag_exceptions)
 	  continue;
-	}
 
-      /* If there are no exceptions, then any noreturn call must have
-	 zero successor edges.  Otherwise, check for blocks without
-	 non-EH successors, but skip those with resx stmts and edges
-	 (i.e., those other than that in bb_eh_cleanup), since those
-	 will go through bb_eh_cleanup, that will have been counted as
-	 noreturn above because it has no successors.  */
-      gcc_checking_assert (bb != bb_eh_cleanup);
-      if (!flag_exceptions)
-	continue;
-
-      bool found_non_eh_edge = false;
-      bool found_eh_edge = false;
-      edge e;
-      edge_iterator ei;
-      FOR_EACH_EDGE (e, ei, bb->succs)
-	{
-	  if ((e->flags & EDGE_EH))
-	    found_eh_edge = true;
-	  else
-	    found_non_eh_edge = true;
-	  if (found_non_eh_edge && found_eh_edge)
-	    break;
-	}
+	bool found_non_eh_edge = false;
+	bool found_eh_edge = false;
+	edge e;
+	edge_iterator ei;
+	FOR_EACH_EDGE (e, ei, bb->succs)
+	  {
+	    if ((e->flags & EDGE_EH))
+	      found_eh_edge = true;
+	    else
+	      found_non_eh_edge = true;
+	    if (found_non_eh_edge && found_eh_edge)
+	      break;
+	  }
 
-      if (found_non_eh_edge)
-	continue;
+	if (found_non_eh_edge)
+	  continue;
 
-      if (found_eh_edge)
-	{
-	  /* We don't wish to check before (re?)raises, those will
-	     have checking performed at bb_eh_cleanup.  The one
-	     exception is bb_eh_cleanup itself.  */
-	  gimple_stmt_iterator gsi = gsi_last_bb (bb);
-	  gcc_checking_assert (!gsi_end_p (gsi));
-	  gimple *stmt = gsi_stmt (gsi);
-	  if (is_a <gresx *> (stmt))
-	    continue;
-	}
+	if (found_eh_edge)
+	  {
+	    /* We don't wish to check before (re?)raises, those will
+	       have checking performed at bb_eh_cleanup.  The one
+	       exception is bb_eh_cleanup itself.  */
+	    gimple_stmt_iterator gsi = gsi_last_bb (bb);
+	    gcc_checking_assert (!gsi_end_p (gsi));
+	    gimple *stmt = gsi_stmt (gsi);
+	    if (is_a <gresx *> (stmt))
+	      continue;
+	  }
 
-      if (bitmap_set_bit (noreturn_blocks, bb->index))
-	count_noreturn++;
-    }
+	if (bitmap_set_bit (noreturn_blocks, bb->index))
+	  count_noreturn++;
+      }
+  else if (bb_eh_cleanup
+	   && bitmap_set_bit (noreturn_blocks, bb_eh_cleanup->index))
+    count_noreturn++;
 
   gcc_checking_assert (!bb_eh_cleanup
 		       || bitmap_bit_p (noreturn_blocks, bb_eh_cleanup->index));
@@ -873,8 +907,19 @@ pass_harden_control_flow_redundancy::execute (function *fun)
 
   rt_bb_visited vstd (count_noreturn);
 
-  FOR_EACH_BB_FN (bb, fun)
-    vstd.visit (bb);
+  /* Visit blocks in index order, because building rtcfg depends on
+     that.  Blocks must be compact, which the cleanup_cfg requirement
+     ensures.  This would also enable FOR_EACH_BB_FN to be used to
+     iterate in index order, but bb_eh_cleanup block splits and
+     insertions changes that.  We could run cfg_cleanup.  */
+  gcc_checking_assert (n_basic_blocks_for_fn (fun)
+		       == last_basic_block_for_fn (fun));
+  for (int i = NUM_FIXED_BLOCKS; i < n_basic_blocks_for_fn (fun); i++)
+    {
+      bb = BASIC_BLOCK_FOR_FN (fun, i);
+      gcc_checking_assert (bb->index == i);
+      vstd.visit (bb, bitmap_bit_p (noreturn_blocks, i));
+    }
 
   vstd.check (count_noreturn, noreturn_blocks);
 
diff --git a/libgcc/hardcfr.c b/libgcc/hardcfr.c
index 8ef29428111..0bf2599fd15 100644
--- a/libgcc/hardcfr.c
+++ b/libgcc/hardcfr.c
@@ -60,14 +60,25 @@ extern void __hardcfr_check (size_t blocks,
 			     vword const *visited,
 			     vword const *cfg);
 
+/* Compute the MASK for the bit representing BLOCK in WORDIDX's vword in a
+   visited blocks bit array.  */
+static inline void
+block2mask (size_t const block, vword *const mask, size_t *const wordidx)
+{
+  size_t wbits = __CHAR_BIT__ * sizeof (vword);
+  *wordidx = block / wbits;
+  *mask = (vword)1 << (block % wbits);
+}
 
 /* Check whether the bit corresponding to BLOCK is set in VISITED.  */
 static inline bool
 visited_p (size_t const block, vword const *const visited)
 {
-  size_t wbits = __CHAR_BIT__ * sizeof (vword);
-  vword w = visited[block / wbits];
-  return (w & ((vword)1 << (block % wbits))) != 0;
+  vword mask;
+  size_t wordidx;
+  block2mask (block, &mask, &wordidx);
+  vword w = visited[wordidx];
+  return (w & mask) != 0;
 }
 
 /* Read and consume a mask from **CFG_IT.  (Consume meaning advancing the
@@ -118,19 +129,19 @@ consume_seq (vword const **const cfg_it)
    we reach the terminator without finding any.  Consume the entire sequence
    otherwise, so that *CFG_IT points just past the terminator, which may be the
    beginning of the next sequence.  */
-static inline void
+static inline bool
 check_seq (vword const *const visited, vword const **const cfg_it)
 {
   vword mask;
   size_t wordidx;
 
   /* If the block was visited, check that at least one of the
-     preds was also visited.  */
+     preds/succs was also visited.  */
   do
     /* If we get to the end of the sequence without finding any
        match, something is amiss.  */
     if (!next_pair (cfg_it, &mask, &wordidx))
-      __builtin_trap ();
+      return false;
   /* Keep searching until we find a match, at which point the
      condition is satisfied.  */
   while (!test_mask (visited, mask, wordidx));
@@ -139,6 +150,84 @@ check_seq (vword const *const visited, vword const **const cfg_it)
      skipped the block, so as to position the iterator at the beginning of the
      next .  */
   consume_seq (cfg_it);
+
+  return true;
+}
+
+static inline void
+__hardcfr_debug_cfg (size_t const blocks,
+		     void *caller,
+		     vword const *const cfg)
+{
+  __builtin_printf ("CFG at %p, for %p", cfg, caller);
+  vword const *cfg_it = cfg;
+  for (size_t i = 0; i < blocks; i++)
+    {
+      vword mask; size_t wordidx;
+      block2mask (i, &mask, &wordidx);
+      __builtin_printf ("\nblock %lu (%lu/0x%lx)\npreds: ",
+			(unsigned long)i,
+			(unsigned long)wordidx, (unsigned long)mask);
+      while (next_pair (&cfg_it, &mask, &wordidx))
+	__builtin_printf (" (%lu/0x%lx)",
+			  (unsigned long)wordidx, (unsigned long)mask);
+      __builtin_printf ("\nsuccs: ");
+      while (next_pair (&cfg_it, &mask, &wordidx))
+	__builtin_printf (" (%lu/0x%lx)",
+			  (unsigned long)wordidx, (unsigned long)mask);
+    }
+  __builtin_printf ("\n");
+}
+
+#ifndef ATTRIBUTE_UNUSED
+# define ATTRIBUTE_UNUSED __attribute__ ((__unused__))
+#endif
+
+static inline void
+__hardcfr_check_fail (size_t const blocks ATTRIBUTE_UNUSED,
+		      vword const *const visited,
+		      vword const *const cfg ATTRIBUTE_UNUSED,
+		      size_t block ATTRIBUTE_UNUSED,
+		      int which ATTRIBUTE_UNUSED,
+		      void *caller ATTRIBUTE_UNUSED)
+{
+#if 1
+  static const char *parts[] = { "preds", "succs" };
+
+  vword mask; size_t wordidx;
+  block2mask (block, &mask, &wordidx);
+  __builtin_printf ("hardcfr fail at %p block %lu (%lu/0x%lx), expected %s:",
+		    caller, (unsigned long)block,
+		    (unsigned long)wordidx, (unsigned long)mask,
+		    parts[which]);
+
+  /* Skip data for previous blocks.  */
+  vword const *cfg_it = cfg;
+  for (size_t i = block; i--; )
+    {
+      consume_seq (&cfg_it);
+      consume_seq (&cfg_it);
+    }
+  for (size_t i = which; i--; )
+    consume_seq (&cfg_it);
+  
+  while (next_pair (&cfg_it, &mask, &wordidx))
+    __builtin_printf (" (%lu/0x%lx)",
+		      (unsigned long)wordidx, (unsigned long)mask);
+
+  __builtin_printf ("\nvisited:");
+  block2mask (blocks, &mask, &wordidx);
+  for (size_t i = 0; i <= wordidx; i++)
+    __builtin_printf (" (%lu/0x%lx)",
+		      (unsigned long)i, (unsigned long)visited[i]);
+  __builtin_printf ("\n");
+
+  /* Reference __hardcfr_debug_cfg so that it's output out-of-line, so that it
+     can be called from a debugger.  */
+  if (!caller || caller == __hardcfr_debug_cfg)
+    return;
+#endif
+  __builtin_trap ();
 }
 
 /* Check that, for each of the BLOCKS basic blocks, if its bit is set in
@@ -168,9 +257,13 @@ __hardcfr_check (size_t const blocks,
       else
 	{
 	  /* Check predecessors.  */
-	  check_seq (visited, &cfg_it);
+	  if (!check_seq (visited, &cfg_it))
+	    __hardcfr_check_fail (blocks, visited, cfg, i, 0,
+				  __builtin_return_address (0));
 	  /* Check successors.  */
-	  check_seq (visited, &cfg_it);
+	  if (!check_seq (visited, &cfg_it))
+	    __hardcfr_check_fail (blocks, visited, cfg, i, 1,
+				  __builtin_return_address (0));
 	}
     }
 }