From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6553 invoked by alias); 5 Dec 2018 17:25:13 -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 6369 invoked by uid 89); 5 Dec 2018 17:25:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 05 Dec 2018 17:25:09 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5525AC058CB0; Wed, 5 Dec 2018 17:25:06 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-68.rdu2.redhat.com [10.10.112.68]) by smtp.corp.redhat.com (Postfix) with ESMTP id 32E2C600C4; Wed, 5 Dec 2018 17:25:04 +0000 (UTC) Subject: Re: [PATCH][RFC] Poison bitmap_head->obstack To: Richard Biener Cc: gcc-patches@gcc.gnu.org References: <71dde62a-ef0b-0bd9-cbc0-1cdb140d56ae@redhat.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: <82405d88-c2b8-11b5-4bf4-4d16111b2e93@redhat.com> Date: Wed, 05 Dec 2018 17:25:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg00298.txt.bz2 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. jeff