From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 7146C385840F for ; Fri, 24 Feb 2023 10:59:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7146C385840F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677236399; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=Rugt96gMaY6TRKRol+biMOkciTWDuTqaIGYHC5Kte7U=; b=F/kwvBhqubMAzJ9gLsW6HOXrZ/lLYDwuuH/W3zfFZSaj2KR9sBauDxfHOfs4vBQ1o/iqnT 9Ed/bkcgXQPIsr5R6b3WMbz24FMA1W3PnMOiRyRrWlotseEgmND+c5a282oL9SsaG6tEt/ bqncBuxROIddFmEZ1W1taHQAOKwG4oU= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-633-1x5Z1DoOMvqZhr3CKIyOTA-1; Fri, 24 Feb 2023 05:59:57 -0500 X-MC-Unique: 1x5Z1DoOMvqZhr3CKIyOTA-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7256F3C0E200; Fri, 24 Feb 2023 10:59:57 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.62]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 164F52166B29; Fri, 24 Feb 2023 10:59:56 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 31OAxsId863201 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 24 Feb 2023 11:59:55 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 31OAxrM4863200; Fri, 24 Feb 2023 11:59:53 +0100 Date: Fri, 24 Feb 2023 11:59:53 +0100 From: Jakub Jelinek To: Jonathan Wakely Cc: Richard Biener , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Avoid default-initializing auto_vec storage Message-ID: Reply-To: Jakub Jelinek References: MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, Feb 24, 2023 at 10:30:00AM +0000, Jonathan Wakely wrote: > On Fri, 24 Feb 2023 at 10:24, Jakub Jelinek wrote: > > > > On Fri, Feb 24, 2023 at 11:02:07AM +0100, Jakub Jelinek via Gcc-patches wrote: > > > Maybe this would work, vl_relative even could be vl_embed. > > > Because vl_embed I believe is used in two spots, part of > > > auto_vec where it is followed by m_data and on heap or GGC > > > allocated memory where vec<..., vl_embed> is followed by > > > further storage for the vector. > > > > So roughtly something like below? Except I get weird crashes with it > > in the gen* tools. And we'd need to adjust the gdb python hooks > > which also use m_vecdata. > > > > --- gcc/vec.h.jj 2023-01-02 09:32:32.177143804 +0100 > > +++ gcc/vec.h 2023-02-24 11:19:37.900157177 +0100 > > @@ -586,8 +586,8 @@ public: > > unsigned allocated (void) const { return m_vecpfx.m_alloc; } > > unsigned length (void) const { return m_vecpfx.m_num; } > > bool is_empty (void) const { return m_vecpfx.m_num == 0; } > > - T *address (void) { return m_vecdata; } > > - const T *address (void) const { return m_vecdata; } > > + T *address (void) { return (T *) (this + 1); } > > + const T *address (void) const { return (const T *) (this + 1); } > > T *begin () { return address (); } > > const T *begin () const { return address (); } > > T *end () { return address () + length (); } > > @@ -629,10 +629,9 @@ public: > > friend struct va_gc_atomic; > > friend struct va_heap; > > > > - /* FIXME - These fields should be private, but we need to cater to > > + /* FIXME - This field should be private, but we need to cater to > > compilers that have stricter notions of PODness for types. */ > > - vec_prefix m_vecpfx; > > - T m_vecdata[1]; > > + alignas (T) alignas (vec_prefix) vec_prefix m_vecpfx; > > }; > > > > > > @@ -879,7 +878,7 @@ inline const T & > > vec::operator[] (unsigned ix) const > > { > > gcc_checking_assert (ix < m_vecpfx.m_num); > > - return m_vecdata[ix]; > > + return address ()[ix]; > > } > > > > template > > @@ -887,7 +886,7 @@ inline T & > > vec::operator[] (unsigned ix) > > { > > gcc_checking_assert (ix < m_vecpfx.m_num); > > - return m_vecdata[ix]; > > + return address ()[ix]; > > } > > > > > > @@ -929,7 +928,7 @@ vec::iterate (unsigned i > > { > > if (ix < m_vecpfx.m_num) > > { > > - *ptr = m_vecdata[ix]; > > + *ptr = address ()[ix]; > > return true; > > } > > else > > @@ -955,7 +954,7 @@ vec::iterate (unsigned i > > { > > if (ix < m_vecpfx.m_num) > > { > > - *ptr = CONST_CAST (T *, &m_vecdata[ix]); > > + *ptr = CONST_CAST (T *, &address ()[ix]); > > return true; > > } > > else > > @@ -978,7 +977,7 @@ vec::copy (ALONE_MEM_STA > > { > > vec_alloc (new_vec, len PASS_MEM_STAT); > > new_vec->embedded_init (len, len); > > - vec_copy_construct (new_vec->address (), m_vecdata, len); > > + vec_copy_construct (new_vec->address (), address (), len); > > } > > return new_vec; > > } > > @@ -1018,7 +1017,7 @@ inline T * > > vec::quick_push (const T &obj) > > { > > gcc_checking_assert (space (1)); > > - T *slot = &m_vecdata[m_vecpfx.m_num++]; > > + T *slot = &address ()[m_vecpfx.m_num++]; > > *slot = obj; > > return slot; > > } > > @@ -1031,7 +1030,7 @@ inline T & > > vec::pop (void) > > { > > gcc_checking_assert (length () > 0); > > - return m_vecdata[--m_vecpfx.m_num]; > > + return address ()[--m_vecpfx.m_num]; > > } > > > > > > @@ -1056,7 +1055,7 @@ vec::quick_insert (unsig > > { > > gcc_checking_assert (length () < allocated ()); > > gcc_checking_assert (ix <= length ()); > > - T *slot = &m_vecdata[ix]; > > + T *slot = &address ()[ix]; > > memmove (slot + 1, slot, (m_vecpfx.m_num++ - ix) * sizeof (T)); > > *slot = obj; > > } > > @@ -1071,7 +1070,7 @@ inline void > > vec::ordered_remove (unsigned ix) > > { > > gcc_checking_assert (ix < length ()); > > - T *slot = &m_vecdata[ix]; > > + T *slot = &address ()[ix]; > > memmove (slot, slot + 1, (--m_vecpfx.m_num - ix) * sizeof (T)); > > } > > > > @@ -1118,7 +1117,7 @@ inline void > > vec::unordered_remove (unsigned ix) > > { > > gcc_checking_assert (ix < length ()); > > - m_vecdata[ix] = m_vecdata[--m_vecpfx.m_num]; > > + address ()[ix] = address ()[--m_vecpfx.m_num]; > > } > > > > > > @@ -1130,7 +1129,7 @@ inline void > > vec::block_remove (unsigned ix, unsigned len) > > { > > gcc_checking_assert (ix + len <= length ()); > > - T *slot = &m_vecdata[ix]; > > + T *slot = &address ()[ix]; > > m_vecpfx.m_num -= len; > > memmove (slot, slot + len, (m_vecpfx.m_num - ix) * sizeof (T)); > > } > > @@ -1309,7 +1308,7 @@ vec::embedded_size (unsi > > vec, vec_embedded>::type vec_stdlayout; > > static_assert (sizeof (vec_stdlayout) == sizeof (vec), ""); > > static_assert (alignof (vec_stdlayout) == alignof (vec), ""); > > - return offsetof (vec_stdlayout, m_vecdata) + alloc * sizeof (T); > > + return sizeof (vec_stdlayout) + alloc * sizeof (T); > > } > > > > > > @@ -1476,10 +1475,10 @@ public: > > { return m_vec ? m_vec->length () : 0; } > > > > T *address (void) > > - { return m_vec ? m_vec->m_vecdata : NULL; } > > + { return m_vec ? m_vec->address () : NULL; } > > > > const T *address (void) const > > - { return m_vec ? m_vec->m_vecdata : NULL; } > > + { return m_vec ? m_vec->address () : NULL; } > > > > T *begin () { return address (); } > > const T *begin () const { return address (); } > > @@ -1584,7 +1583,7 @@ public: > > > > private: > > vec m_auto; > > - T m_data[MAX (N - 1, 1)]; > > + unsigned char m_data[MAX (N - 1, 1)]; > > This needs to be alignas(T) unsigned char m_data[sizeof(T) * N]; unsigned char m_data[MAX (N, 2) * sizeof (T)]; if we want to preserve current behavior I think. I've screwed up, when I was about to change this line, I've realized I want to look at the embedded_size stuff first and then forgot to update this. With the above line it builds, though I bet one will need to compare the generated code etc. and test with GCC 4.8. Though I guess we now have also an option to change auto_vec with N 0/1 to have 1 embedded element instead of 2, so that would also mean changing all of MAX (N, 2) to MAX (N, 1) if we want. --- gcc/vec.h.jj 2023-01-02 09:32:32.177143804 +0100 +++ gcc/vec.h 2023-02-24 11:54:49.859564268 +0100 @@ -586,8 +586,8 @@ public: unsigned allocated (void) const { return m_vecpfx.m_alloc; } unsigned length (void) const { return m_vecpfx.m_num; } bool is_empty (void) const { return m_vecpfx.m_num == 0; } - T *address (void) { return m_vecdata; } - const T *address (void) const { return m_vecdata; } + T *address (void) { return (T *) (this + 1); } + const T *address (void) const { return (const T *) (this + 1); } T *begin () { return address (); } const T *begin () const { return address (); } T *end () { return address () + length (); } @@ -629,10 +629,9 @@ public: friend struct va_gc_atomic; friend struct va_heap; - /* FIXME - These fields should be private, but we need to cater to + /* FIXME - This field should be private, but we need to cater to compilers that have stricter notions of PODness for types. */ - vec_prefix m_vecpfx; - T m_vecdata[1]; + alignas (T) alignas (vec_prefix) vec_prefix m_vecpfx; }; @@ -879,7 +878,7 @@ inline const T & vec::operator[] (unsigned ix) const { gcc_checking_assert (ix < m_vecpfx.m_num); - return m_vecdata[ix]; + return address ()[ix]; } template @@ -887,7 +886,7 @@ inline T & vec::operator[] (unsigned ix) { gcc_checking_assert (ix < m_vecpfx.m_num); - return m_vecdata[ix]; + return address ()[ix]; } @@ -929,7 +928,7 @@ vec::iterate (unsigned i { if (ix < m_vecpfx.m_num) { - *ptr = m_vecdata[ix]; + *ptr = address ()[ix]; return true; } else @@ -955,7 +954,7 @@ vec::iterate (unsigned i { if (ix < m_vecpfx.m_num) { - *ptr = CONST_CAST (T *, &m_vecdata[ix]); + *ptr = CONST_CAST (T *, &address ()[ix]); return true; } else @@ -978,7 +977,7 @@ vec::copy (ALONE_MEM_STA { vec_alloc (new_vec, len PASS_MEM_STAT); new_vec->embedded_init (len, len); - vec_copy_construct (new_vec->address (), m_vecdata, len); + vec_copy_construct (new_vec->address (), address (), len); } return new_vec; } @@ -1018,7 +1017,7 @@ inline T * vec::quick_push (const T &obj) { gcc_checking_assert (space (1)); - T *slot = &m_vecdata[m_vecpfx.m_num++]; + T *slot = &address ()[m_vecpfx.m_num++]; *slot = obj; return slot; } @@ -1031,7 +1030,7 @@ inline T & vec::pop (void) { gcc_checking_assert (length () > 0); - return m_vecdata[--m_vecpfx.m_num]; + return address ()[--m_vecpfx.m_num]; } @@ -1056,7 +1055,7 @@ vec::quick_insert (unsig { gcc_checking_assert (length () < allocated ()); gcc_checking_assert (ix <= length ()); - T *slot = &m_vecdata[ix]; + T *slot = &address ()[ix]; memmove (slot + 1, slot, (m_vecpfx.m_num++ - ix) * sizeof (T)); *slot = obj; } @@ -1071,7 +1070,7 @@ inline void vec::ordered_remove (unsigned ix) { gcc_checking_assert (ix < length ()); - T *slot = &m_vecdata[ix]; + T *slot = &address ()[ix]; memmove (slot, slot + 1, (--m_vecpfx.m_num - ix) * sizeof (T)); } @@ -1118,7 +1117,7 @@ inline void vec::unordered_remove (unsigned ix) { gcc_checking_assert (ix < length ()); - m_vecdata[ix] = m_vecdata[--m_vecpfx.m_num]; + address ()[ix] = address ()[--m_vecpfx.m_num]; } @@ -1130,7 +1129,7 @@ inline void vec::block_remove (unsigned ix, unsigned len) { gcc_checking_assert (ix + len <= length ()); - T *slot = &m_vecdata[ix]; + T *slot = &address ()[ix]; m_vecpfx.m_num -= len; memmove (slot, slot + len, (m_vecpfx.m_num - ix) * sizeof (T)); } @@ -1309,7 +1308,7 @@ vec::embedded_size (unsi vec, vec_embedded>::type vec_stdlayout; static_assert (sizeof (vec_stdlayout) == sizeof (vec), ""); static_assert (alignof (vec_stdlayout) == alignof (vec), ""); - return offsetof (vec_stdlayout, m_vecdata) + alloc * sizeof (T); + return sizeof (vec_stdlayout) + alloc * sizeof (T); } @@ -1476,10 +1475,10 @@ public: { return m_vec ? m_vec->length () : 0; } T *address (void) - { return m_vec ? m_vec->m_vecdata : NULL; } + { return m_vec ? m_vec->address () : NULL; } const T *address (void) const - { return m_vec ? m_vec->m_vecdata : NULL; } + { return m_vec ? m_vec->address () : NULL; } T *begin () { return address (); } const T *begin () const { return address (); } @@ -1584,7 +1583,7 @@ public: private: vec m_auto; - T m_data[MAX (N - 1, 1)]; + unsigned char m_data[MAX (N, 2) * sizeof (T)]; }; /* auto_vec is a sub class of vec whose storage is released when it is Jakub