From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75363 invoked by alias); 6 Dec 2018 23:38:56 -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 75245 invoked by uid 89); 6 Dec 2018 23:38:55 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=*head, (unknown) X-HELO: mail-qk1-f196.google.com Received: from mail-qk1-f196.google.com (HELO mail-qk1-f196.google.com) (209.85.222.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 06 Dec 2018 23:38:53 +0000 Received: by mail-qk1-f196.google.com with SMTP id y16so1453365qki.7 for ; Thu, 06 Dec 2018 15:38:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=gn1nPN+HYnMXMcrpqeX1uwTDHiESz+BOGXbcc4GKd/4=; b=jBdXZxBphb1NUGmUApeXblHBaD9MT7FO9oZy38Y/unjb40LrQerR/NUr2Yb3fPtw8i M+PvSGKpNCAJK7cuE/2VXftewtm9HzkksmrzsjlB7JoHNgoD61rgrghLsbmaK+WC06Xc w6fyykqppj9hMvNuovx+2OtfiqEnQULg5B/vyGqAw5eqaRlEIEir6Okm4lXLgE5nae07 jtHcXLwWbdtfsQWjOv/HsvJ5yGmS6ZlNpCftE6HVPS6bhDHpqT8UGt9YJ5EJy3YmslL2 CKtaSc6/7cIr4J6QKjWTg68mvacvo9jxkKQ7dcPLvt6ysnAKqOHWwjZB8J5np2zyfi2y uHTw== Return-Path: Received: from localhost.localdomain (97-118-99-160.hlrn.qwest.net. [97.118.99.160]) by smtp.gmail.com with ESMTPSA id x19sm979987qtj.5.2018.12.06.15.38.49 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 06 Dec 2018 15:38:50 -0800 (PST) Subject: Re: [PATCH][RFC] Poison bitmap_head->obstack To: Richard Biener , Jeff Law Cc: gcc-patches@gcc.gnu.org References: <71dde62a-ef0b-0bd9-cbc0-1cdb140d56ae@redhat.com> From: Martin Sebor Message-ID: Date: Thu, 06 Dec 2018 23:38:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg00421.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... 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. >