[PATCH][RFC] Poison bitmap_head->obstack

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH][RFC] Poison bitmap_head->obstack

Richard Biener

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  <[hidden email]>

        * 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.

Index: gcc/bitmap.c
===================================================================
--- gcc/bitmap.c (revision 266772)
+++ gcc/bitmap.c (working copy)
@@ -26,6 +26,10 @@ along with GCC; see the file COPYING3.
 /* Memory allocation statistics purpose instance.  */
 mem_alloc_description<bitmap_usage> bitmap_mem_desc;
 
+/* Static zero-initialized bitmap obstack used for default initialization
+   of bitmap_head.  */
+bitmap_obstack bitmap_head::crashme;
+
 static bitmap_element *bitmap_tree_listify_from (bitmap, bitmap_element *);
 
 /* Register new bitmap.  */
Index: gcc/bitmap.h
===================================================================
--- gcc/bitmap.h (revision 266772)
+++ gcc/bitmap.h (working copy)
@@ -323,6 +323,12 @@ struct GTY((chain_next ("%h.next"), chai
    already pointed to by the chain started by first, so GTY((skip)) it.  */
 
 struct GTY(()) bitmap_head {
+  static bitmap_obstack crashme;
+  /* Poison obstack to not make it not a valid initialized GC bitmap.  */
+  CONSTEXPR bitmap_head()
+    : indx(0), tree_form(false), first(NULL), current(NULL),
+      obstack (&crashme)
+  {}
   /* Index of last element looked at.  */
   unsigned int indx;
   /* False if the bitmap is in list form; true if the bitmap is in tree form.
@@ -441,6 +446,18 @@ bitmap_initialize (bitmap head, bitmap_o
     bitmap_register (head PASS_MEM_STAT);
 }
 
+/* Release a bitmap (but not its head).  This is suitable for pairing with
+   bitmap_initialize.  */
+
+static inline void
+bitmap_release (bitmap head)
+{
+  bitmap_clear (head);
+  /* Poison the obstack pointer so the obstack can be safely released.
+     Do not zero it as the bitmap then becomes initialized GC.  */
+  head->obstack = (bitmap_obstack *)0xdeadbeef;
+}
+
 /* Allocate and free bitmaps from obstack, malloc and gc'd memory.  */
 extern bitmap bitmap_alloc (bitmap_obstack *obstack CXX_MEM_STAT_INFO);
 #define BITMAP_ALLOC bitmap_alloc
Index: gcc/gengtype.c
===================================================================
--- gcc/gengtype.c (revision 266772)
+++ gcc/gengtype.c (working copy)
@@ -5205,6 +5205,7 @@ main (int argc, char **argv)
       POS_HERE (do_scalar_typedef ("void", &pos));
       POS_HERE (do_scalar_typedef ("machine_mode", &pos));
       POS_HERE (do_scalar_typedef ("fixed_size_mode", &pos));
+      POS_HERE (do_scalar_typedef ("CONSTEXPR", &pos));
       POS_HERE (do_typedef ("PTR",
     create_pointer (resolve_typedef ("void", &pos)),
     &pos));
Index: gcc/ira.c
===================================================================
--- gcc/ira.c (revision 266772)
+++ gcc/ira.c (working copy)
@@ -5417,7 +5417,7 @@ ira (FILE *f)
     = ((struct ira_spilled_reg_stack_slot *)
        ira_allocate (max_regno
      * sizeof (struct ira_spilled_reg_stack_slot)));
-  memset (ira_spilled_reg_stack_slots, 0,
+  memset ((void *)ira_spilled_reg_stack_slots, 0,
   max_regno * sizeof (struct ira_spilled_reg_stack_slot));
  }
     }
Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c (revision 266772)
+++ gcc/lra-constraints.c (working copy)
@@ -6647,11 +6647,11 @@ lra_inheritance (void)
    inherit_in_ebb.  */
  update_ebb_live_info (BB_HEAD (start_bb), BB_END (bb));
     }
-  bitmap_clear (&ebb_global_regs);
-  bitmap_clear (&temp_bitmap);
-  bitmap_clear (&live_regs);
-  bitmap_clear (&invalid_invariant_regs);
-  bitmap_clear (&check_only_regs);
+  bitmap_release (&ebb_global_regs);
+  bitmap_release (&temp_bitmap);
+  bitmap_release (&live_regs);
+  bitmap_release (&invalid_invariant_regs);
+  bitmap_release (&check_only_regs);
   free (usage_insns);
 
   timevar_pop (TV_LRA_INHERITANCE);
Index: gcc/regrename.c
===================================================================
--- gcc/regrename.c (revision 266772)
+++ gcc/regrename.c (working copy)
@@ -231,7 +231,7 @@ create_new_chain (unsigned this_regno, u
   struct du_chain *this_du;
   int nregs;
 
-  memset (head, 0, sizeof *head);
+  memset ((void *)head, 0, sizeof *head);
   head->next_chain = open_chains;
   head->regno = this_regno;
   head->nregs = this_nregs;
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][RFC] Poison bitmap_head->obstack

Jeff Law
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  <[hidden email]>
>
> * 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.

jeff
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][RFC] Poison bitmap_head->obstack

Richard Biener
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  <[hidden email]>
> >
> > * 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...

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...

Thanks,
Richard.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][RFC] Poison bitmap_head->obstack

Jeff Law
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  <[hidden email]>
>>>
>>> * 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.

jeff
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][RFC] Poison bitmap_head->obstack

Richard Biener
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  <[hidden email]>
> >>>
> >>> * 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 <[hidden email]>
Date: Thu, 6 Dec 2018 10:28:30 +0100
Subject: [PATCH] bitmap-poison-followup

2018-12-06  Richard Biener  <[hidden email]>

        * 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 ();
 
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][RFC] Poison bitmap_head->obstack

Martin Sebor-2
In reply to this post by Richard Biener
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  <[hidden email]>
>>>
>>> * 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...

Using '*head = du_head ();' is the solution I like to encourage
to zero out typed objects.  When the memory isn't initialized
and the type has a user-defined ctor, bypassing it is undefined
even if the ctor is a no-op (the ctor starts the lifetime of
the object).  In that case, placement new is the appropriate
way to bring the object to life and value-initialize it:

   new (head) du_head ();

If that's not good enough or portable enough to ancient compilers
then I would suggest adding a comment to explain the intent of
the cast.

Martin

>
> 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...
>
> Thanks,
> Richard.
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][RFC] Poison bitmap_head->obstack

Richard Biener
On Thu, 6 Dec 2018, Martin Sebor 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  <[hidden email]>
> > > >
> > > > * 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...
>
> Using '*head = du_head ();' is the solution I like to encourage
> to zero out typed objects.  When the memory isn't initialized
> and the type has a user-defined ctor, bypassing it is undefined
> even if the ctor is a no-op (the ctor starts the lifetime of
> the object).  In that case, placement new is the appropriate
> way to bring the object to life and value-initialize it:
>
>   new (head) du_head ();
>
> If that's not good enough or portable enough to ancient compilers
> then I would suggest adding a comment to explain the intent of
> the cast.

Yes, I know.  But we have workarounds like the following and I
didn't want to open up another hole just for this debugging feature

#ifdef BROKEN_VALUE_INITIALIZATION
  /* Versions of GCC before 4.4 sometimes leave certain objects
     uninitialized when value initialized, though if the type has
     user defined default ctor, that ctor is invoked.  As a workaround
     perform clearing first and then the value initialization, which
     fixes the case when value initialization doesn't initialize due to
     the bugs and should initialize to all zeros, but still allows
     vectors for types with user defined default ctor that initializes
     some or all elements to non-zero.  If T has no user defined
     default ctor and some non-static data members have user defined
     default ctors that initialize to non-zero the workaround will
     still not work properly; in that case we just need to provide
     user defined default ctor.  */
  memset (dst, '\0', sizeof (T) * n);
#endif
  for ( ; n; ++dst, --n)
    ::new (static_cast<void*>(dst)) T ();

Richard.

> Martin
>
> >
> > 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...
> >
> > Thanks,
> > Richard.
> >
>
>

--
Richard Biener <[hidden email]>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH][RFC] Poison bitmap_head->obstack

Martin Sebor-2
On 12/7/18 1:10 AM, Richard Biener wrote:

> On Thu, 6 Dec 2018, Martin Sebor 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  <[hidden email]>
>>>>>
>>>>> * 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...
>>
>> Using '*head = du_head ();' is the solution I like to encourage
>> to zero out typed objects.  When the memory isn't initialized
>> and the type has a user-defined ctor, bypassing it is undefined
>> even if the ctor is a no-op (the ctor starts the lifetime of
>> the object).  In that case, placement new is the appropriate
>> way to bring the object to life and value-initialize it:
>>
>>    new (head) du_head ();
>>
>> If that's not good enough or portable enough to ancient compilers
>> then I would suggest adding a comment to explain the intent of
>> the cast.
>
> Yes, I know.  But we have workarounds like the following and I
> didn't want to open up another hole just for this debugging feature
>
> #ifdef BROKEN_VALUE_INITIALIZATION
>    /* Versions of GCC before 4.4 sometimes leave certain objects
>       uninitialized when value initialized, though if the type has
>       user defined default ctor, that ctor is invoked.  As a workaround
>       perform clearing first and then the value initialization, which
>       fixes the case when value initialization doesn't initialize due to
>       the bugs and should initialize to all zeros, but still allows
>       vectors for types with user defined default ctor that initializes
>       some or all elements to non-zero.  If T has no user defined
>       default ctor and some non-static data members have user defined
>       default ctors that initialize to non-zero the workaround will
>       still not work properly; in that case we just need to provide
>       user defined default ctor.  */
>    memset (dst, '\0', sizeof (T) * n);
> #endif
>    for ( ; n; ++dst, --n)
>      ::new (static_cast<void*>(dst)) T ();

Why not then introduce a couple of helpers to do it without
duplicating the workaround each time we find ourselves faced
with this problem?  Something like this perhaps:

   template <class T>
   inline void value_init (void *p, size_t n)
   {
   #ifndef BROKEN_VALUE_INITIALIZATION
     for (T *q = static_cast<T*> (p); n; --n, ++q)
       ::new (q) T();
   #else
      memset (p, 0, n * sizeof (T));
   #endif
   }

   template <class T>
   inline void value_assign (T *p, size_t n)
   {
   #ifndef BROKEN_VALUE_INITIALIZATION
     for ( ; n; --n, ++p)
       *p = T ();
   #else
      memset (p, 0, n * sizeof (T));
   #endif
   }

That way the code will be clean and portable at the same time,
and the workarounds easy to find and remove if we choose once
the broken compilers are no longer supported.

Martin

>
> Richard.
>
>> Martin
>>
>>>
>>> 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...
>>>
>>> Thanks,
>>> Richard.
>>>
>>
>>
>