| Message ID | 20240201160400.2143624-1-jwakely@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] libstdc++: Replace padding bits with a bit-field in __format::_Spec | expand |
> From: Jonathan Wakely <jwakely@redhat.com> > Cc: Hans-Peter Nilsson <hp@axis.com> > Date: Thu, 1 Feb 2024 15:36:50 +0000 > I plan to push this to trunk soon. > > CC HP for visibility of the change affecting cris-elf. In practice it > shouldn't make any difference to any sensible code. It only affects > C++20 mode (and later), and only changes the size of std::formatter > objects which are typically only created by the library headers > themselves, and only on the stack (when using std::format and other new > C++20 APIs related to it). > > -- >8 -- > > This ensures that the unused bits will be zero-initialized reliably, and > so can be used later by assigning them values in formatter > specializations. For example, formatters for std::chrono will need to > use an extra bit for a boolean to optimize the conversions between > locale encodings and UTF-8. > > This will result in an ABI change for targets that use 1-byte alignment > for all integral types, e.g. cris-elf. Thanks for the heads-up, but don't worry about ABI changes for cris-elf. Not speaking for other platforms with default-packed layout or where ABI structure layout alignment implies a change due to PCC_BITFIELD_TYPE_MATTERS and the "unsigned long" bitfield type. That last one may matter though. > We can't do that once C++20 > support is non-experimental and ABI stable, so do it now before GCC 14 > is released. > > libstdc++-v3/ChangeLog: > > * include/std/format (__format::_Spec::_M_reserved): Define a > new bit-field member in place of padding bits. > --- > libstdc++-v3/include/std/format | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libstdc++-v3/include/std/format b/libstdc++-v3/include/std/format > index 0eca8b58bfa..6c958bc11a5 100644 > --- a/libstdc++-v3/include/std/format > +++ b/libstdc++-v3/include/std/format > @@ -406,6 +406,7 @@ namespace __format > _WidthPrec _M_width_kind : 2; > _WidthPrec _M_prec_kind : 2; > _Pres_type _M_type : 4; > + unsigned long _M_reserved : 17; > unsigned short _M_width; > unsigned short _M_prec; > char32_t _M_fill = ' '; > -- > 2.43.0 >
On Feb 01 2024, Jonathan Wakely wrote: > This will result in an ABI change for targets that use 1-byte alignment > for all integral types, e.g. cris-elf. Or 2-byte alignment as on m68k.
> From: Hans-Peter Nilsson <hp@axis.com> > Date: Thu, 1 Feb 2024 17:16:47 +0100 > Not speaking for other platforms with default-packed layout > or where ABI structure layout alignment implies a change due > to PCC_BITFIELD_TYPE_MATTERS and the "unsigned long" > bitfield type. > > That last one may matter though. > > diff --git a/libstdc++-v3/include/std/format b/libstdc++-v3/include/std/format > > index 0eca8b58bfa..6c958bc11a5 100644 > > --- a/libstdc++-v3/include/std/format > > +++ b/libstdc++-v3/include/std/format > > @@ -406,6 +406,7 @@ namespace __format > > _WidthPrec _M_width_kind : 2; > > _WidthPrec _M_prec_kind : 2; > > _Pres_type _M_type : 4; > > + unsigned long _M_reserved : 17; FAOD (no doubt you got this already, but...) I'd suggest making that "unsigned long" only for 16-bitters (i.e. where you'd need a type larger than "unsigned int" to cover 17 bits) and "unsigned" elsewhere, so at least you have no impact from PCC_BITFIELD_TYPE_MATTERS. brgds, H-P
On Thu, 1 Feb 2024 at 17:08, Hans-Peter Nilsson <hp@axis.com> wrote: > > > From: Hans-Peter Nilsson <hp@axis.com> > > Date: Thu, 1 Feb 2024 17:16:47 +0100 > > > Not speaking for other platforms with default-packed layout > > or where ABI structure layout alignment implies a change due > > to PCC_BITFIELD_TYPE_MATTERS and the "unsigned long" > > bitfield type. > > > > That last one may matter though. > > > > diff --git a/libstdc++-v3/include/std/format b/libstdc++-v3/include/std/format > > > index 0eca8b58bfa..6c958bc11a5 100644 > > > --- a/libstdc++-v3/include/std/format > > > +++ b/libstdc++-v3/include/std/format > > > @@ -406,6 +406,7 @@ namespace __format > > > _WidthPrec _M_width_kind : 2; > > > _WidthPrec _M_prec_kind : 2; > > > _Pres_type _M_type : 4; > > > + unsigned long _M_reserved : 17; > > FAOD (no doubt you got this already, but...) > > I'd suggest making that "unsigned long" only for 16-bitters (i.e. where > you'd need a type larger than "unsigned int" to cover 17 bits) and > "unsigned" elsewhere, so at least you have no impact from > PCC_BITFIELD_TYPE_MATTERS. Ah, I have never heard of that. I guess using uint32_t would be OK.
On Thu, 1 Feb 2024 at 16:34, Andreas Schwab <schwab@suse.de> wrote: > > On Feb 01 2024, Jonathan Wakely wrote: > > > This will result in an ABI change for targets that use 1-byte alignment > > for all integral types, e.g. cris-elf. > > Or 2-byte alignment as on m68k. Ah yes. In fact it's a change for everybody, because previously it was 15 bits of bit-fields, 1 bit padding, 2 x 16-bit short, 16 bits padding, then a char32_t, but now it's 15+17 bits of bit-fields, 2x 16-bit short, char32_t. So the shorts moved internally even if the size didn't change. I could make it 15+1 bit-fields, 2x16-bit short, 16 bit-field to avoid moving the shorts. template<typename _CharT> struct _Spec { _Align _M_align : 2; _Sign _M_sign : 2; unsigned _M_alt : 1; unsigned _M_localized : 1; unsigned _M_zero_fill : 1; _WidthPrec _M_width_kind : 2; _WidthPrec _M_prec_kind : 2; _Pres_type _M_type : 4; unsigned _M_reserved : 1; unsigned short _M_width; unsigned short _M_prec; unsigned _M_reserved2 : 16; char32_t _M_fill = ' '; That would also address the PCC_BITFIELD_TYPE_MATTERS point H-P made.
On Thu, 1 Feb 2024, 17:23 Jonathan Wakely, <jwakely@redhat.com> wrote: > On Thu, 1 Feb 2024 at 16:34, Andreas Schwab <schwab@suse.de> wrote: > > > > On Feb 01 2024, Jonathan Wakely wrote: > > > > > This will result in an ABI change for targets that use 1-byte alignment > > > for all integral types, e.g. cris-elf. > > > > Or 2-byte alignment as on m68k. > > Ah yes. > > In fact it's a change for everybody, because previously it was 15 bits > of bit-fields, 1 bit padding, 2 x 16-bit short, 16 bits padding, (Except for targets with 1- or 2-byte alignment, which didn't have the second padding) then > a char32_t, but now it's 15+17 bits of bit-fields, 2x 16-bit short, > char32_t. So the shorts moved internally even if the size didn't > change. > > I could make it 15+1 bit-fields, 2x16-bit short, 16 bit-field to avoid > moving the shorts. > > template<typename _CharT> > struct _Spec > { > _Align _M_align : 2; > _Sign _M_sign : 2; > unsigned _M_alt : 1; > unsigned _M_localized : 1; > unsigned _M_zero_fill : 1; > _WidthPrec _M_width_kind : 2; > _WidthPrec _M_prec_kind : 2; > _Pres_type _M_type : 4; > unsigned _M_reserved : 1; > unsigned short _M_width; > unsigned short _M_prec; > unsigned _M_reserved2 : 16; > char32_t _M_fill = ' '; > > That would also address the PCC_BITFIELD_TYPE_MATTERS point H-P made. > >
On Thu, 1 Feb 2024 at 17:22, Jonathan Wakely <jwakely@redhat.com> wrote: > > On Thu, 1 Feb 2024 at 16:34, Andreas Schwab <schwab@suse.de> wrote: > > > > On Feb 01 2024, Jonathan Wakely wrote: > > > > > This will result in an ABI change for targets that use 1-byte alignment > > > for all integral types, e.g. cris-elf. > > > > Or 2-byte alignment as on m68k. > > Ah yes. > > In fact it's a change for everybody, because previously it was 15 bits > of bit-fields, 1 bit padding, 2 x 16-bit short, 16 bits padding, then > a char32_t, but now it's 15+17 bits of bit-fields, 2x 16-bit short, > char32_t. So the shorts moved internally even if the size didn't > change. > > I could make it 15+1 bit-fields, 2x16-bit short, 16 bit-field to avoid > moving the shorts. > > template<typename _CharT> > struct _Spec > { > _Align _M_align : 2; > _Sign _M_sign : 2; > unsigned _M_alt : 1; > unsigned _M_localized : 1; > unsigned _M_zero_fill : 1; > _WidthPrec _M_width_kind : 2; > _WidthPrec _M_prec_kind : 2; > _Pres_type _M_type : 4; > unsigned _M_reserved : 1; > unsigned short _M_width; > unsigned short _M_prec; > unsigned _M_reserved2 : 16; > char32_t _M_fill = ' '; > > That would also address the PCC_BITFIELD_TYPE_MATTERS point H-P made. Oh, and I forgot to mention in the first email that I already changed the ABI of that type in r14-6991-g37a4c5c23a270c so it's already different from gcc-13 anyway. I think I'd prefer to keep the reserved bits together, but a simpler way to avoid 'unsigned long' making a difference for PCC_BITFIELD_TYPE_MATTERS targets would be to use no more than 16 bits but do: unsigned _M_reserved : 1; unsigned _M_reserved2 : 16;
> From: Jonathan Wakely <jwakely@redhat.com> > Date: Thu, 1 Feb 2024 19:24:49 +0000 > I think I'd prefer to keep the reserved bits together, but a simpler > way to avoid 'unsigned long' making a difference for > PCC_BITFIELD_TYPE_MATTERS targets would be to use no more than 16 bits > but do: > > unsigned _M_reserved : 1; > unsigned _M_reserved2 : 16; > Hah! Genious! :-) brgds, H-P
diff --git a/libstdc++-v3/include/std/format b/libstdc++-v3/include/std/format index 0eca8b58bfa..6c958bc11a5 100644 --- a/libstdc++-v3/include/std/format +++ b/libstdc++-v3/include/std/format @@ -406,6 +406,7 @@ namespace __format _WidthPrec _M_width_kind : 2; _WidthPrec _M_prec_kind : 2; _Pres_type _M_type : 4; + unsigned long _M_reserved : 17; unsigned short _M_width; unsigned short _M_prec; char32_t _M_fill = ' ';