From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24851 invoked by alias); 7 Dec 2018 08:10:30 -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 24685 invoked by uid 89); 7 Dec 2018 08:10:20 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_PASS autolearn=ham version=3.3.2 spammy= 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; Fri, 07 Dec 2018 08:10:18 +0000 Received: from relay1.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id D64EEABEF; Fri, 7 Dec 2018 08:10:15 +0000 (UTC) Date: Fri, 07 Dec 2018 08:10:00 -0000 From: Richard Biener To: Martin Sebor cc: Jeff Law , gcc-patches@gcc.gnu.org Subject: Re: [PATCH][RFC] Poison bitmap_head->obstack In-Reply-To: Message-ID: References: <71dde62a-ef0b-0bd9-cbc0-1cdb140d56ae@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/msg00433.txt.bz2 On Thu, 6 Dec 2018, Martin Sebor 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... > > 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. Yes, I know. But we have workarounds like the following and I didn't want to open up another hole just for this debugging feature #ifdef BROKEN_VALUE_INITIALIZATION /* Versions of GCC before 4.4 sometimes leave certain objects uninitialized when value initialized, though if the type has user defined default ctor, that ctor is invoked. As a workaround perform clearing first and then the value initialization, which fixes the case when value initialization doesn't initialize due to the bugs and should initialize to all zeros, but still allows vectors for types with user defined default ctor that initializes some or all elements to non-zero. If T has no user defined default ctor and some non-static data members have user defined default ctors that initialize to non-zero the workaround will still not work properly; in that case we just need to provide user defined default ctor. */ memset (dst, '\0', sizeof (T) * n); #endif for ( ; n; ++dst, --n) ::new (static_cast(dst)) T (); Richard. > 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. > > > > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)