[PATCH][RFC] Poison bitmap_head->obstack

Richard Biener rguenther@suse.de
Thu Dec 6 09:36:00 GMT 2018


On Wed, 5 Dec 2018, Jeff Law wrote:

> On 12/5/18 7:58 AM, Richard Biener wrote:
> > On Wed, 5 Dec 2018, Jeff Law wrote:
> > 
> >> On 12/4/18 6:16 AM, Richard Biener wrote:
> >>>
> >>> This tries to make bugs like that in PR88317 harder to create by
> >>> introducing a bitmap_release function that can be used as
> >>> pendant to bitmap_initialize for non-allocated bitmap heads.
> >>> The function makes sure to poison the bitmaps obstack member
> >>> so the obstack the bitmap was initialized with can be safely
> >>> released.
> >>>
> >>> The patch also adds a default constructor to bitmap_head
> >>> doing the same, but for C++ reason initializes to a
> >>> all-zero bitmap_obstack rather than 0xdeadbeef because
> >>> the latter isn't possible in constexpr context (it is
> >>> by using unions but then things start to look even more ugly).
> >>>
> >>> The stage1 compiler might end up with a few extra runtime
> >>> initializers but constexpr makes sure they'll vanish for
> >>> later stages.
> >>>
> >>> I had to paper over that you-may-not-use-memset-to-zero classes
> >>> with non-trivial constructors warning in two places and I
> >>> had to teach gengtype about CONSTEXPR (probably did so in
> >>> an awkward way - suggestions and pointers into gengtype
> >>> appreciated).
> >>>
> >>> Bootstrapped (with host GCC 4.8) on x86_64-unknown-linux-gnu,
> >>> testing in progress.
> >>>
> >>> The LRA issue seems to be rare enough (on x86_64...) that
> >>> I didn't trip over it sofar.
> >>>
> >>> Comments?  Do we want this?  Not sure how we can easily
> >>> discover all bitmap_clear () users that should really
> >>> use bitmap_release (suggestion for a better name appreciated
> >>> as well - I thought about bitmap_uninitialize)
> >>>
> >>> Richard.
> >>>
> >>> 2018-12-04  Richard Biener  <rguenther@suse.de>
> >>>
> >>> 	* bitmap.c (bitmap_head::crashme): Define.
> >>> 	* bitmap.h (bitmap_head): Add constexpr default constructor
> >>> 	poisoning the obstack member.
> >>> 	(bitmap_head::crashme): Declare.
> >>> 	(bitmap_release): New function clearing a bitmap and poisoning
> >>> 	the obstack member.
> >>> 	* gengtype.c (main): Make it recognize CONSTEXPR.
> >>>
> >>> 	* lra-constraints.c (lra_inheritance): Use bitmap_release
> >>> 	instead of bitmap_clear.
> >>>
> >>> 	* ira.c (ira): Work around warning.
> >>> 	* regrename.c (create_new_chain): Likewise.
> >> I don't see enough complexity in here to be concerning -- so if it makes
> >> it harder to make mistakes, then I'm for it.
> > 
> > Any comment about the -Wclass-memaccess workaround sprinkling around two
> > (void *) conversions?  I didn't dig deep enough to look for a more
> > appropriate solution, also because there were some issues with older
> > host compilers and workarounds we installed elsewhere...
> Not really.  It was just a couple casts and a normal looking ctor, so it
> didn't seem terrible.  Someone with more C++-fu may have a better
> suggestion, but it seemed reasonable to me.
> 
> > 
> > Otherwise yes, it makes it harder to do mistakes.  I'll probably
> > use bitmap_head::crashme instead of 0xdeadbeef in bitmap_release.
> > And of course we'd need to hunt down users of bitmap_clear that
> > should be bitmap_release instead...
> Right, but when we trip this kind of thing we'll know to starting
> digging around the bitmap_clear calls :-)  That's a huge head start.

OK.  I'll commit the patch later today then.

Currently testing with the following followup after I adjusted
all 'static bitmap_head ' vars in gcc/*.c.  I noticed some
oddities there, like using GC allocation for such bitmaps but
the bitmaps being not marked GTY (and being short-lived), and
sel-sched.c exporting a bitmap_head via a pointer, not using
the bitmap_head directly anywhere.

Bootstrap & regtest running on x86_64-unknown-linux-gnu.

>From 6c90c1c10f0f91a7a37feadd4f583ed8aaf5bcc7 Mon Sep 17 00:00:00 2001
From: Richard Guenther <rguenther@suse.de>
Date: Thu, 6 Dec 2018 10:28:30 +0100
Subject: [PATCH] bitmap-poison-followup

2018-12-06  Richard Biener  <rguenther@suse.de>

	* df-problems.c (df_rd_local_compute): Use bitmap_release.
	(df_live_free): Likewise.
	(df_md_local_compute): Likewise.
	(df_md_free): Release df_md_scratch bitmap.
	* loop-invariant.c (calculate_loop_reg_pressure): Use
	bitmap_release.
	* sched-deps.c (true_dependency_cache, output_dependency_cache,
	anti_dependency_cache, control_dependency_cache,
	spec_dependency_cache): Use bitmap instead of bitmap_head *.
	* sched-ebb.c (schedule_ebbs_init): Initialize non-GTY
	dont_calc_deps as bitmap allocated from obstack not GC.
	(schedule_ebbs_finish): Use bitmap_release.
	* sched-rgn.c (schedule_insns): Initialize non-GTY
	not_in_df as bitmap allocated from obstack not GC.
	Use bitmap_release.
	* sel-sched.c (_forced_ebb_heads): Remove premature optimization.
	(sel_region_init): Allocate forced_ebb_heads.
	(sel_region_finish): Free forced_ebb_heads.

diff --git a/gcc/df-problems.c b/gcc/df-problems.c
index 7ccb57c287a..ccab9a96bd7 100644
--- a/gcc/df-problems.c
+++ b/gcc/df-problems.c
@@ -419,8 +419,8 @@ df_rd_local_compute (bitmap all_blocks)
 	}
     }
 
-  bitmap_clear (&seen_in_block);
-  bitmap_clear (&seen_in_insn);
+  bitmap_release (&seen_in_block);
+  bitmap_release (&seen_in_insn);
 }
 
 
@@ -1585,7 +1585,7 @@ df_live_free (void)
       df_live->block_info_size = 0;
       free (df_live->block_info);
       df_live->block_info = NULL;
-      bitmap_clear (&df_live_scratch);
+      bitmap_release (&df_live_scratch);
       bitmap_obstack_release (&problem_data->live_bitmaps);
       free (problem_data);
       df_live->problem_data = NULL;
@@ -4533,7 +4533,7 @@ df_md_local_compute (bitmap all_blocks)
       df_md_bb_local_compute (bb_index);
     }
 
-  bitmap_clear (&seen_in_insn);
+  bitmap_release (&seen_in_insn);
 
   frontiers = XNEWVEC (bitmap_head, last_basic_block_for_fn (cfun));
   FOR_ALL_BB_FN (bb, cfun)
@@ -4649,6 +4649,7 @@ df_md_free (void)
   struct df_md_problem_data *problem_data
     = (struct df_md_problem_data *) df_md->problem_data;
 
+  bitmap_release (&df_md_scratch);
   bitmap_obstack_release (&problem_data->md_bitmaps);
   free (problem_data);
   df_md->problem_data = NULL;
diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c
index e3b2eda1695..5bd6fc771ee 100644
--- a/gcc/loop-invariant.c
+++ b/gcc/loop-invariant.c
@@ -2201,7 +2201,7 @@ calculate_loop_reg_pressure (void)
 	    }
 	}
     }
-  bitmap_clear (&curr_regs_live);
+  bitmap_release (&curr_regs_live);
   if (flag_ira_region == IRA_REGION_MIXED
       || flag_ira_region == IRA_REGION_ALL)
     FOR_EACH_LOOP (loop, 0)
diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index f89f28269fd..dfdf5cc8895 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -461,11 +461,11 @@ static HARD_REG_SET implicit_reg_pending_uses;
    has enough entries to represent a dependency on any other insn in
    the insn chain.  All bitmap for true dependencies cache is
    allocated then the rest two ones are also allocated.  */
-static bitmap_head *true_dependency_cache = NULL;
-static bitmap_head *output_dependency_cache = NULL;
-static bitmap_head *anti_dependency_cache = NULL;
-static bitmap_head *control_dependency_cache = NULL;
-static bitmap_head *spec_dependency_cache = NULL;
+static bitmap true_dependency_cache = NULL;
+static bitmap output_dependency_cache = NULL;
+static bitmap anti_dependency_cache = NULL;
+static bitmap control_dependency_cache = NULL;
+static bitmap spec_dependency_cache = NULL;
 static int cache_size;
 
 /* True if we should mark added dependencies as a non-register deps.  */
diff --git a/gcc/sched-ebb.c b/gcc/sched-ebb.c
index c3be0e33855..49ae2865419 100644
--- a/gcc/sched-ebb.c
+++ b/gcc/sched-ebb.c
@@ -588,15 +588,14 @@ schedule_ebbs_init (void)
   compute_bb_for_insn ();
 
   /* Initialize DONT_CALC_DEPS and ebb-{start, end} markers.  */
-  bitmap_initialize (&dont_calc_deps, 0);
-  bitmap_clear (&dont_calc_deps);
+  bitmap_initialize (&dont_calc_deps, &bitmap_default_obstack);
 }
 
 /* Perform cleanups after scheduling using schedules_ebbs or schedule_ebb.  */
 void
 schedule_ebbs_finish (void)
 {
-  bitmap_clear (&dont_calc_deps);
+  bitmap_release (&dont_calc_deps);
 
   /* Reposition the prologue and epilogue notes in case we moved the
      prologue/epilogue insns.  */
diff --git a/gcc/sched-rgn.c b/gcc/sched-rgn.c
index 3c67fccb9b1..ea8dd5c7b76 100644
--- a/gcc/sched-rgn.c
+++ b/gcc/sched-rgn.c
@@ -3507,8 +3507,7 @@ schedule_insns (void)
   haifa_sched_init ();
   sched_rgn_init (reload_completed);
 
-  bitmap_initialize (&not_in_df, 0);
-  bitmap_clear (&not_in_df);
+  bitmap_initialize (&not_in_df, &bitmap_default_obstack);
 
   /* Schedule every region in the subroutine.  */
   for (rgn = 0; rgn < nr_regions; rgn++)
@@ -3517,7 +3516,7 @@ schedule_insns (void)
 
   /* Clean up.  */
   sched_rgn_finish ();
-  bitmap_clear (&not_in_df);
+  bitmap_release (&not_in_df);
 
   haifa_sched_finish ();
 }
diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index 824f1ec3403..e57a8f2dcef 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -473,8 +473,7 @@ static int first_emitted_uid;
 
 /* Set of basic blocks that are forced to start new ebbs.  This is a subset
    of all the ebb heads.  */
-static bitmap_head _forced_ebb_heads;
-bitmap_head *forced_ebb_heads = &_forced_ebb_heads;
+bitmap forced_ebb_heads;
 
 /* Blocks that need to be rescheduled after pipelining.  */
 bitmap blocks_to_reschedule = NULL;
@@ -6947,8 +6946,7 @@ sel_region_init (int rgn)
   memset (reg_rename_tick, 0, sizeof reg_rename_tick);
   reg_rename_this_tick = 0;
 
-  bitmap_initialize (forced_ebb_heads, 0);
-  bitmap_clear (forced_ebb_heads);
+  forced_ebb_heads = BITMAP_ALLOC (NULL);
 
   setup_nop_vinsn ();
   current_copies = BITMAP_ALLOC (NULL);
@@ -7290,7 +7288,7 @@ sel_region_finish (bool reset_sched_cycles_p)
 
   sel_finish_global_and_expr ();
 
-  bitmap_clear (forced_ebb_heads);
+  BITMAP_FREE (forced_ebb_heads);
 
   free_nop_vinsn ();
 



More information about the Gcc-patches mailing list