From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 95943 invoked by alias); 29 Apr 2016 08:06:35 -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 95921 invoked by uid 89); 29 Apr 2016 08:06:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=Thoughts X-HELO: mail-wm0-f53.google.com Received: from mail-wm0-f53.google.com (HELO mail-wm0-f53.google.com) (74.125.82.53) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 29 Apr 2016 08:06:32 +0000 Received: by mail-wm0-f53.google.com with SMTP id v200so8775078wmv.1 for ; Fri, 29 Apr 2016 01:06:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=13hZZLiggQ9RPB3Bj/m0sjbYCe8w4OrD2gQlm0RM/Ww=; b=E1R2Vs3+WC8CfpZHjXceiDisCynQ1GANwIPl6ju0aDpiheH/PFfT3Nx62dO6DxHiRn 7Ngvhjry/MED6M4u5SY44j8JpbZ4lqCtAveGIUHJPR9UaZob5vjdtOK3youOTIyQpqXM TLmzLBlsWbMSEOA34yyeN41xQ+AZPH76JLKHPl73fdHxreF4wwSlfZMa5ES3QzLt/WWG hh6TD6JZo8zlMTeC77LibHwGsXNM2OzN98wFz5qc2jFc4tVenCIGG1uJa9dMUHTQMJy2 JTEes8cLhAXWo2LPwpbS7Q75fzvAaIjsx7pqybXSYQlLzmwi6G6x1HtblPSoeeOcE+Nk bbqw== X-Gm-Message-State: AOPr4FUaAKis4/9X7wEKPLFpPK2EnSN3X1MCE6LO75Ql8ZMFENY4jVIsOMezy6G4i720xIbTE7SlP7LRSLJ4Kw== MIME-Version: 1.0 X-Received: by 10.28.154.2 with SMTP id c2mr2368519wme.9.1461917189558; Fri, 29 Apr 2016 01:06:29 -0700 (PDT) Received: by 10.194.113.102 with HTTP; Fri, 29 Apr 2016 01:06:29 -0700 (PDT) In-Reply-To: References: <56F0461C.3070601@redhat.com> <56F0470F.6080804@redhat.com> <56F05A74.2080200@redhat.com> Date: Fri, 29 Apr 2016 08:06:00 -0000 Message-ID: Subject: Re: [RFA][PATCH] Adding missing calls to bitmap_clear From: Richard Biener To: Jeff Law Cc: Bernd Schmidt , GCC Patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2016-04/txt/msg01939.txt.bz2 On Thu, Apr 28, 2016 at 9:35 PM, Jeff Law wrote: > On 03/22/2016 03:37 AM, Richard Biener wrote: >> >> On Mon, Mar 21, 2016 at 9:32 PM, Jeff Law 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 > >