[2/2] Avoid default-initializing auto_vec<T, N> storage, fix vec<vl_embed>
Commit Message
The following avoids default-initializing auto_vec storage for
non-POD T since that's not what the allocated storage fallback
will do and it's also not expected for existing cases like
auto_vec<std::pair<unsigned, unsigned>, 64> elts;
which exist to optimize the allocation.
It also fixes the array accesses done by vec<vl_embed> to not
use its own m_vecdata member but instead access the container
provided storage via pointer arithmetic.
I've built the series with GCC 4.8 and clang 13 up to the stage1
target libs, a bootstrap and regtest on x86_64-unknown-linux-gnu
with GCC 12 was successful with the diagnostic pragma, I'm
currently re-bootstrapping and testing with a GCC 7 host compiler.
OK if that succeeds?
Thanks,
Richard.
* vec.h (vec<T, A, vl_embed>::m_vecdata): Remove.
(vec<T, A, vl_embed>::m_vecpfx): Align as T to avoid
changing alignment of vec<T, A, vl_embed> and simplifying
address.
(vec<T, A, vl_embed>::address): Compute as this + 1.
(vec<T, A, vl_embed>::embedded_size): Use sizeof the
vector instead of the offset of the m_vecdata member.
(auto_vec<T, N>::m_data): Turn storage into
uninitialized unsigned char.
(auto_vec<T, N>::auto_vec): Allow allocation of one
stack member. Initialize m_vec in a special way to
avoid later stringop overflow diagnostics.
* vec.cc (test_auto_alias): New.
(vec_cc_tests): Call it.
---
gcc/vec.cc | 17 +++++++++++++++++
gcc/vec.h | 27 +++++++++++++++++----------
2 files changed, 34 insertions(+), 10 deletions(-)
Comments
On Fri, Feb 24, 2023 at 02:47:39PM +0100, Richard Biener wrote:
> * vec.h (vec<T, A, vl_embed>::m_vecdata): Remove.
> (vec<T, A, vl_embed>::m_vecpfx): Align as T to avoid
> changing alignment of vec<T, A, vl_embed> and simplifying
> address.
> (vec<T, A, vl_embed>::address): Compute as this + 1.
> (vec<T, A, vl_embed>::embedded_size): Use sizeof the
> vector instead of the offset of the m_vecdata member.
> (auto_vec<T, N>::m_data): Turn storage into
> uninitialized unsigned char.
> (auto_vec<T, N>::auto_vec): Allow allocation of one
> stack member. Initialize m_vec in a special way to
> avoid later stringop overflow diagnostics.
> * vec.cc (test_auto_alias): New.
> (vec_cc_tests): Call it.
> @@ -1559,8 +1560,14 @@ class auto_vec : public vec<T, va_heap>
> public:
> auto_vec ()
> {
> - m_auto.embedded_init (MAX (N, 2), 0, 1);
> - this->m_vec = &m_auto;
> + m_auto.embedded_init (N, 0, 1);
> + /* ??? Instead of initializing m_vec from &m_auto directly use an
> + expression that avoids refering to a specific member of 'this'
> + to derail the -Wstringop-overflow diagnostic code, avoiding
> + the impression that data accesses are supposed to be to the
> + m_auto memmber storage. */
s/memmber/member/
> + size_t off = (char *) &m_auto - (char *) this;
> + this->m_vec = (vec<T, va_heap, vl_embed> *) ((char *) this + off);
> }
>
> auto_vec (size_t s CXX_MEM_STAT_INFO)
> @@ -1571,7 +1578,7 @@ public:
> return;
> }
>
> - m_auto.embedded_init (MAX (N, 2), 0, 1);
> + m_auto.embedded_init (N, 0, 1);
> this->m_vec = &m_auto;
Don't we need the above 2 lines here as well (perhaps with a shorter comment
just referencing the earlier comment)?
Otherwise LGTM, thanks.
Jakub
On Fri, 24 Feb 2023, Jakub Jelinek wrote:
> On Fri, Feb 24, 2023 at 02:47:39PM +0100, Richard Biener wrote:
> > * vec.h (vec<T, A, vl_embed>::m_vecdata): Remove.
> > (vec<T, A, vl_embed>::m_vecpfx): Align as T to avoid
> > changing alignment of vec<T, A, vl_embed> and simplifying
> > address.
> > (vec<T, A, vl_embed>::address): Compute as this + 1.
> > (vec<T, A, vl_embed>::embedded_size): Use sizeof the
> > vector instead of the offset of the m_vecdata member.
> > (auto_vec<T, N>::m_data): Turn storage into
> > uninitialized unsigned char.
> > (auto_vec<T, N>::auto_vec): Allow allocation of one
> > stack member. Initialize m_vec in a special way to
> > avoid later stringop overflow diagnostics.
> > * vec.cc (test_auto_alias): New.
> > (vec_cc_tests): Call it.
> > @@ -1559,8 +1560,14 @@ class auto_vec : public vec<T, va_heap>
> > public:
> > auto_vec ()
> > {
> > - m_auto.embedded_init (MAX (N, 2), 0, 1);
> > - this->m_vec = &m_auto;
> > + m_auto.embedded_init (N, 0, 1);
> > + /* ??? Instead of initializing m_vec from &m_auto directly use an
> > + expression that avoids refering to a specific member of 'this'
> > + to derail the -Wstringop-overflow diagnostic code, avoiding
> > + the impression that data accesses are supposed to be to the
> > + m_auto memmber storage. */
>
> s/memmber/member/
>
> > + size_t off = (char *) &m_auto - (char *) this;
> > + this->m_vec = (vec<T, va_heap, vl_embed> *) ((char *) this + off);
> > }
> >
> > auto_vec (size_t s CXX_MEM_STAT_INFO)
> > @@ -1571,7 +1578,7 @@ public:
> > return;
> > }
> >
> > - m_auto.embedded_init (MAX (N, 2), 0, 1);
> > + m_auto.embedded_init (N, 0, 1);
> > this->m_vec = &m_auto;
>
> Don't we need the above 2 lines here as well (perhaps with a shorter comment
> just referencing the earlier comment)?
I've noticed that as well and put it there now, it wasn't necessary
to get bootstrap working.
> Otherwise LGTM, thanks.
Thanks,
Richard.
@@ -568,6 +568,22 @@ test_auto_delete_vec ()
ASSERT_EQ (dtor_count, 2);
}
+/* Verify accesses to m_vecdata are done indirectly. */
+
+static void
+test_auto_alias ()
+{
+ volatile int i = 1;
+ auto_vec<int, 8> v;
+ v.quick_grow (2);
+ v[0] = 1;
+ v[1] = 2;
+ int val;
+ for (int ix = i; v.iterate (ix, &val); ix++)
+ ASSERT_EQ (val, 2);
+ ASSERT_EQ (val, 0);
+}
+
/* Run all of the selftests within this file. */
void
@@ -587,6 +603,7 @@ vec_cc_tests ()
test_qsort ();
test_reverse ();
test_auto_delete_vec ();
+ test_auto_alias ();
}
} // namespace selftest
@@ -586,8 +586,9 @@ 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 reinterpret_cast <T *> (this + 1); }
+ const T *address (void) const
+ { return reinterpret_cast <const T *> (this + 1); }
T *begin () { return address (); }
const T *begin () const { return address (); }
T *end () { return address () + length (); }
@@ -629,10 +630,10 @@ 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];
+ /* Align m_vecpfx to simplify address (). */
+ alignas (T) alignas (vec_prefix) vec_prefix m_vecpfx;
};
@@ -1315,7 +1316,7 @@ vec<T, A, vl_embed>::embedded_size (unsigned alloc)
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);
}
@@ -1559,8 +1560,14 @@ class auto_vec : public vec<T, va_heap>
public:
auto_vec ()
{
- m_auto.embedded_init (MAX (N, 2), 0, 1);
- this->m_vec = &m_auto;
+ m_auto.embedded_init (N, 0, 1);
+ /* ??? Instead of initializing m_vec from &m_auto directly use an
+ expression that avoids refering to a specific member of 'this'
+ to derail the -Wstringop-overflow diagnostic code, avoiding
+ the impression that data accesses are supposed to be to the
+ m_auto memmber storage. */
+ size_t off = (char *) &m_auto - (char *) this;
+ this->m_vec = (vec<T, va_heap, vl_embed> *) ((char *) this + off);
}
auto_vec (size_t s CXX_MEM_STAT_INFO)
@@ -1571,7 +1578,7 @@ public:
return;
}
- m_auto.embedded_init (MAX (N, 2), 0, 1);
+ m_auto.embedded_init (N, 0, 1);
this->m_vec = &m_auto;
}
@@ -1590,7 +1597,7 @@ public:
private:
vec<T, va_heap, vl_embed> m_auto;
- T m_data[MAX (N - 1, 1)];
+ unsigned char m_data[sizeof (T) * N];
};
/* auto_vec is a sub class of vec whose storage is released when it is