On Thu, Apr 28, 2016 at 9:35 PM, Jeff Law <l...@redhat.com> wrote: > On 03/22/2016 03:37 AM, Richard Biener wrote: >> >> On Mon, Mar 21, 2016 at 9:32 PM, Jeff Law <l...@redhat.com> wrote: >>> >>> On 03/21/2016 01:10 PM, Bernd Schmidt wrote: >>>> >>>> >>>> On 03/21/2016 08:06 PM, Jeff Law wrote: >>>>> >>>>> >>>>> >>>>> As noted last week, find_removable_extensions initializes several >>>>> bitmaps, but doesn't clear them. >>>>> >>>>> This is not strictly a leak as the GC system should find dead data, but >>>>> it's better to go ahead and clear the bitmaps. That releases the >>>>> elements back to the cache and presumably makes things easier for the >>>>> GC >>>>> system as well. >>>>> >>>>> Bootstrapped and regression tested on x86_64-linux-gnu. >>>>> >>>>> OK for the trunk? >>>> >>>> >>>> >>>> Looks like they don't leak anywhere, so ok. Probably ok even to install >>>> it now but maybe stage1 would be better timing. >>> >>> >>> I don't mind waiting for the next stage1, this is a pretty minor issue. >> >> >> It's ok at this stage as it will also fix -fmem-report. Please also move >> the thing back to heap, see below. >> >> Btw we should disallow bitmap_initialize (&x, NULL) as it does not do >> the same thing as BITMAP_ALLOC (NULL), it does the same thing >> as BITMAP_ALLOC_GC (). Thus I'd rather have a bitmap_initialize_gc (&x) >> and a bitmap_initialize (&x, NULL) that ends up using the global >> bitmap obstack. No idea where REE came from history wise. >> >> A grep shows only >> >> ira.c: bitmap_initialize (&seen_insns, NULL); >> ree.c: bitmap_initialize (&init, NULL); >> ree.c: bitmap_initialize (&kill, NULL); >> ree.c: bitmap_initialize (&gen, NULL); >> ree.c: bitmap_initialize (&tmp, NULL); > > It's more than that. Sadly folks have passed in "0" instead of NULL in > various places. > > ./haifa-sched.c: bitmap_initialize (&processed, 0); > ./haifa-sched.c: bitmap_initialize (&processed, 0); > ./haifa-sched.c: bitmap_initialize (&in_ready, 0); > ./sched-ebb.c: bitmap_initialize (&dont_calc_deps, 0); > ./sched-rgn.c: bitmap_initialize (¬_in_df, 0); > ./testsuite/gcc.dg/pr45352.c: bitmap_initialize_stat (0); > ./ira.c: bitmap_initialize (&interesting, 0); > ./ira.c: bitmap_initialize (&live, 0); > ./ira.c: bitmap_initialize (&used, 0); > ./ira.c: bitmap_initialize (&set, 0); > ./ira.c: bitmap_initialize (&unusable_as_input, 0); > ./ira.c: bitmap_initialize (local, 0); > ./ira.c: bitmap_initialize (transp, 0); > ./ira.c: bitmap_initialize (moveable, 0); > ./ira.c: bitmap_initialize (&need_new, 0); > ./ira.c: bitmap_initialize (&reachable, 0); > ./sel-sched.c: bitmap_initialize (forced_ebb_heads, 0); > ./sched-deps.c: bitmap_initialize (&true_dependency_cache[i], 0); > ./sched-deps.c: bitmap_initialize (&output_dependency_cache[i], 0); > ./sched-deps.c: bitmap_initialize (&anti_dependency_cache[i], 0); > ./sched-deps.c: bitmap_initialize (&control_dependency_cache[i], 0); > ./sched-deps.c: bitmap_initialize (&spec_dependency_cache[i], 0); > >> >> btw, so please consider simply changing bitmap_initialize behavior. The >> IRA >> use also should use the global bitmap obstack as users around that use >> use BITMAP_ALLOC (NULL). [use a default arg for 'obstack' if possible, >> you have to verify it works with/without >> --enable-gather-detailed-mem-stats] > > The problem is ensuring that allocating off the default bitmap obstack is > appropriate for all those uses.
True, if the bitmap head lives in a GC structure then that's not safe. > I'm tempted to change them all to NULL. Then iterate one by one on to > ensure we're routing to gc vs the default bitmap obstack as appropriate and > that we're calling bitmap_clear as appropriate. > > Once we've fixed all of 'em, we simply assert that bitmap_initialize is > never passed NULL and avoid getting in this situation again in the future. First one sounds good. I'd still add a bitmap_gc_initialize (&head) and change bitmap_initialize (&head, NULL) behavior to match that of BITMAP_ALLOC (NULL). Richard. > Thoughts? > jeff > >
