From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1177 invoked by alias); 18 Jun 2015 11:11:02 -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 1162 invoked by uid 89); 18 Jun 2015 11:11:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ob0-f175.google.com Received: from mail-ob0-f175.google.com (HELO mail-ob0-f175.google.com) (209.85.214.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 18 Jun 2015 11:10:59 +0000 Received: by obbsn1 with SMTP id sn1so51713219obb.1 for ; Thu, 18 Jun 2015 04:10:57 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.60.40.228 with SMTP id a4mr8037899oel.7.1434625857157; Thu, 18 Jun 2015 04:10:57 -0700 (PDT) Received: by 10.76.115.167 with HTTP; Thu, 18 Jun 2015 04:10:57 -0700 (PDT) In-Reply-To: <558187BC.4070603@suse.cz> References: <557E962C.5060501@suse.cz> <557EC3A0.2080303@suse.cz> <557FEBC9.8080002@suse.cz> <558026BB.7060208@suse.cz> <5580416D.8050009@suse.cz> <20150617091358.GC18873@virgil.suse> <20150617112228.GN10247@tucnak.redhat.com> <558187BC.4070603@suse.cz> Date: Thu, 18 Jun 2015 11:18:00 -0000 Message-ID: Subject: Re: New type-based pool allocator code miscompiled due to aliasing issue? From: Richard Biener To: =?UTF-8?Q?Martin_Li=C5=A1ka?= Cc: Jakub Jelinek , GCC Patches Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-06/txt/msg01260.txt.bz2 On Wed, Jun 17, 2015 at 4:44 PM, Martin Li=C5=A1ka wrote: > On 06/17/2015 01:29 PM, Richard Biener wrote: >> On Wed, Jun 17, 2015 at 1:22 PM, Jakub Jelinek wrote: >>> On Wed, Jun 17, 2015 at 11:13:58AM +0200, Martin Jambor wrote: >>>>>>> Do you mean Richard following changes: >>>>>>> >>>>>>> alloc-pool.h (allocate): >>>>>>> ... >>>>>>> + /* Placement new contructor. */ >>>>>>> + inline void *operator new (size_t, elt_loc_list *&ptr) >>>>>>> + { >>>>>>> + return ptr; >>>>>>> + } >>>>>> >>>>>> That should be there with including >>>>>> >>>>>>> and e.g. cselib.h: >>>>>>> >>>>>>> struct cselib_val >>>>>>> { >>>>>>> /* Pool allocation new operator. */ >>>>>>> inline void *operator new (size_t) >>>>>>> { >>>>>>> return pool.allocate (); >>>>>>> } >>>>>>> >>>>>>> /* Placement new contructor. */ >>>>>>> inline void *operator new (size_t, char *&ptr) >>>>>>> { >>>>>>> return ptr; >>>>>>> } >>>>>> >>>>>> Yes, though I wonder whether cselib_val should really have undefined >>>>>> contents after >>>>>> allocating it? (or does the pool allocator zero the memory?) >>> >>> IMHO if you want to put placement new into allocate, then you probably = need >>> two different methods, one that will return you just void * to the >>> allocated memory chunk that you should use >>> inline void *operator new (size_t) { return pool.allocate (); } on, >>> and another method that will use that and additionally invoke a placeme= nt >>> new on it and return you the constructed pointer, which you'd use elsew= here. >>> Otherwise you'd construct the object twice... >> >> Note that in the case of cselib_val a >> >> new cselib_val; >> >> will already invoke the constructor. I think it's just too much >> (partly) C++-ification here. >> Either all alloc-pool users have to work that way or none. >> >> Richard. >> >>> Jakub > > Hello. > > You are right that we should call ::new just for classes that have m_igno= re_type_size =3D=3D false. > I've come up with following patch, that I tested slightly: > > diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h > index 1785df5..7da5f7a 100644 > --- a/gcc/alloc-pool.h > +++ b/gcc/alloc-pool.h > @@ -412,8 +412,16 @@ pool_allocator::allocate () > #endif > VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size)); > > + T *ptr =3D (T *)header; > + > /* Call default constructor. */ > - return (T *)(header); > + if (!m_ignore_type_size) > + { > + memset (header + sizeof (T), 0, m_extra_size); > + return ::new (ptr) T; > + } > + else > + return ptr; > } > > /* Puts PTR back on POOL's free list. */ > > Would it be suitable? Suitable with the memset removed, yes. Thanks, Richard. > Martin