This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][RFC] Poison bitmap_head->obstack
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 (¬_in_df, 0);
- bitmap_clear (¬_in_df);
+ bitmap_initialize (¬_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 (¬_in_df);
+ bitmap_release (¬_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 ();