From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 47996 invoked by alias); 6 Dec 2018 09:36:58 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 45665 invoked by uid 89); 6 Dec 2018 09:36:58 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 spammy=Schedule, regtest, sk:ira_reg, frontiers X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Dec 2018 09:36:52 +0000 Received: from relay1.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 2DE90AFDE; Thu, 6 Dec 2018 09:36:50 +0000 (UTC) Date: Thu, 06 Dec 2018 09:36:00 -0000 From: Richard Biener To: Jeff Law cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH][RFC] Poison bitmap_head->obstack In-Reply-To: <82405d88-c2b8-11b5-4bf4-4d16111b2e93@redhat.com> Message-ID: References: <71dde62a-ef0b-0bd9-cbc0-1cdb140d56ae@redhat.com> <82405d88-c2b8-11b5-4bf4-4d16111b2e93@redhat.com> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2018-12/txt/msg00348.txt.bz2 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 > >>> > >>> * 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 Date: Thu, 6 Dec 2018 10:28:30 +0100 Subject: [PATCH] bitmap-poison-followup 2018-12-06 Richard Biener * 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 ();