On Thu, 23 Feb 2023, Jakub Jelinek wrote:
> On Thu, Feb 23, 2023 at 01:54:27PM +0100, Richard Biener wrote:
> > 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.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >
> > It saves around 1kB of text size for cc1.
> >
> > OK for trunk?
> >
> > Thanks,
> > Richard.
> >
> > * vec.h (auto_vec<T, N>): Turn m_data storage into
> > uninitialized unsigned char.
>
> Given that we actually never reference the m_data array anywhere,
> it is just to reserve space, I think even the alignas(T) there is
> useless. The point is that m_auto has as data members:
> vec_prefix m_vecpfx;
> T m_vecdata[1];
> and we rely on it (admittedly -fstrict-flex-arrays{,=2,=3} or
> -fsanitize=bound-sstrict incompatible) being treated as
> flexible array member flowing into the m_data storage after it.
Doesn't the array otherwise eventually overlap with tail padding
in m_auto? Or does an array of T never produce tail padding?
> Though, this shows we still have work to do. Because
> 1) T m_vecdata[1]; still has element type T in m_auto and
> so is actually constructed/destructed uselessly
True.
> 2) the non-POD support is certainly incomplete; we have
> vec_default_construct and vec_copy_construct and use that
> for say grow_cleared etc. or vector copies, but don't have
> anything that would invoke the destructors and don't invoke
> vec_copy_construct (aka placement new) when we just push
> stuff into the vector; so, I bet what we do is invalid
> and kind of works for non-PODs which have assignment operator
> which overwrites everything, doesn't use anything from the
> overwritten object and has the same overall effect
> as copy constructor, and only support trivial dtors
> For full non-POD support, I bet we'd need to vec_copy_construct (..., 1)
> on quick_push rather than invoke operator= there, we'd need
> to destruct stuff on pop/truncate etc., and quick_grow would need
> to be effectively the same as quick_grow_cleared for non-PODs.
> As for 1), if even m_vecdata was
> alignas(T) unsigned char m_vecdata[sizeof (T)];
> we'll need casts in various spots and it will printing vectors
> by hand in gdb when the .gdbinit printers don't work properly.
Yes, I'm not proposing to fix non-POD support. I want to make
as-if-POD stuff like std::pair to work like it was intended.
> Oh, and perhaps we should start marking such spots in GCC with
> strict_flex_array attribute to make it clear where we rely on the
> non-strict behavior.
I think we never access the array directly as array, do we?
Richard.
