[3/6] libstdc++: port bitwise relocatable away from is_trivial

Hello,

This aligns __is_bitwise_relocatable with its modern meaning, that is, 
checking for trivial move construction and destruction.

Thanks,

On 09/12/24 13:22 +0100, Giuseppe D'Angelo wrote:
>Hello,
>
>This aligns __is_bitwise_relocatable with its modern meaning, that is, 
>checking for trivial move construction and destruction.

Looks good, thanks.

>Thanks,
>-- 
>Giuseppe D'Angelo

>From 0666e993066818ab0940c61d8d9539e883848b29 Mon Sep 17 00:00:00 2001
>From: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
>Date: Mon, 9 Dec 2024 02:11:19 +0100
>Subject: [PATCH 3/6] libstdc++: port bitwise relocatable away from is_trivial
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>In preparation for the deprecation of is_trivial (P3247R2).
>"bitwise relocation" (or "trivial relocation" à la P1144/P2786)
>doesn't need the full-fledged notion of triviality, just checking for a
>trivial move constructor and a trivial destructor is sufficient.
>
>libstdc++-v3/ChangeLog:
>
>	* include/bits/stl_uninitialized.h: Amended the
>	  __is_bitwise_relocatable type trait.
>
>Signed-off-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
>---
> libstdc++-v3/include/bits/stl_uninitialized.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h
>index 2190261134e..916288352d7 100644
>--- a/libstdc++-v3/include/bits/stl_uninitialized.h
>+++ b/libstdc++-v3/include/bits/stl_uninitialized.h
>@@ -1248,7 +1248,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   // Also known as is_trivially_relocatable.
>   template<typename _Tp, typename = void>
>     struct __is_bitwise_relocatable
>-    : is_trivial<_Tp> { };
>+    : __and_<is_trivially_move_constructible<_Tp>, is_trivially_destructible<_Tp>> { };
> 
>   template <typename _InputIterator, typename _ForwardIterator,
> 	    typename _Allocator>
>-- 
>2.34.1
>

On Mon, 9 Dec 2024 at 13:21, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 09/12/24 13:22 +0100, Giuseppe D'Angelo wrote:
> >Hello,
> >
> >This aligns __is_bitwise_relocatable with its modern meaning, that is,
> >checking for trivial move construction and destruction.
>
> Looks good, thanks.
>
> >Thanks,
> >--
> >Giuseppe D'Angelo
>
> >From 0666e993066818ab0940c61d8d9539e883848b29 Mon Sep 17 00:00:00 2001
> >From: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
> >Date: Mon, 9 Dec 2024 02:11:19 +0100
> >Subject: [PATCH 3/6] libstdc++: port bitwise relocatable away from is_trivial
> >MIME-Version: 1.0
> >Content-Type: text/plain; charset=UTF-8
> >Content-Transfer-Encoding: 8bit
> >
> >In preparation for the deprecation of is_trivial (P3247R2).
> >"bitwise relocation" (or "trivial relocation" à la P1144/P2786)
> >doesn't need the full-fledged notion of triviality, just checking for a
> >trivial move constructor and a trivial destructor is sufficient.
> >
> >libstdc++-v3/ChangeLog:
> >
> >       * include/bits/stl_uninitialized.h: Amended the
> >         __is_bitwise_relocatable type trait.
> >
> >Signed-off-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
> >---
> > libstdc++-v3/include/bits/stl_uninitialized.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h
> >index 2190261134e..916288352d7 100644
> >--- a/libstdc++-v3/include/bits/stl_uninitialized.h
> >+++ b/libstdc++-v3/include/bits/stl_uninitialized.h
> >@@ -1248,7 +1248,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >   // Also known as is_trivially_relocatable.
> >   template<typename _Tp, typename = void>
> >     struct __is_bitwise_relocatable
> >-    : is_trivial<_Tp> { };
> >+    : __and_<is_trivially_move_constructible<_Tp>, is_trivially_destructible<_Tp>> { };

We need this to depend on is_trivially_copyable too, so we can use memcpy.

I'm testing a fix now to fix bootstrap.


> >
> >   template <typename _InputIterator, typename _ForwardIterator,
> >           typename _Allocator>
> >--
> >2.34.1
> >
>
>
>

On Tue, 10 Dec 2024 at 09:56, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Mon, 9 Dec 2024 at 13:21, Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> > On 09/12/24 13:22 +0100, Giuseppe D'Angelo wrote:
> > >Hello,
> > >
> > >This aligns __is_bitwise_relocatable with its modern meaning, that is,
> > >checking for trivial move construction and destruction.
> >
> > Looks good, thanks.
> >
> > >Thanks,
> > >--
> > >Giuseppe D'Angelo
> >
> > >From 0666e993066818ab0940c61d8d9539e883848b29 Mon Sep 17 00:00:00 2001
> > >From: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
> > >Date: Mon, 9 Dec 2024 02:11:19 +0100
> > >Subject: [PATCH 3/6] libstdc++: port bitwise relocatable away from is_trivial
> > >MIME-Version: 1.0
> > >Content-Type: text/plain; charset=UTF-8
> > >Content-Transfer-Encoding: 8bit
> > >
> > >In preparation for the deprecation of is_trivial (P3247R2).
> > >"bitwise relocation" (or "trivial relocation" à la P1144/P2786)
> > >doesn't need the full-fledged notion of triviality, just checking for a
> > >trivial move constructor and a trivial destructor is sufficient.
> > >
> > >libstdc++-v3/ChangeLog:
> > >
> > >       * include/bits/stl_uninitialized.h: Amended the
> > >         __is_bitwise_relocatable type trait.
> > >
> > >Signed-off-by: Giuseppe D'Angelo <giuseppe.dangelo@kdab.com>
> > >---
> > > libstdc++-v3/include/bits/stl_uninitialized.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > >diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h
> > >index 2190261134e..916288352d7 100644
> > >--- a/libstdc++-v3/include/bits/stl_uninitialized.h
> > >+++ b/libstdc++-v3/include/bits/stl_uninitialized.h
> > >@@ -1248,7 +1248,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > >   // Also known as is_trivially_relocatable.
> > >   template<typename _Tp, typename = void>
> > >     struct __is_bitwise_relocatable
> > >-    : is_trivial<_Tp> { };
> > >+    : __and_<is_trivially_move_constructible<_Tp>, is_trivially_destructible<_Tp>> { };
>
> We need this to depend on is_trivially_copyable too, so we can use memcpy.

In fact, maybe it should *only* care about is_trivially_copyable. We
already know the type must be move constructible and destructible, or
it wouldn't be usable in std::vector in the first place.

I'm going to revert it to is_trivial with the -Wdeprecated diagnostics
disabled. This isn't an appropriate change for stage 3.


>
> I'm testing a fix now to fix bootstrap.
>
>
> > >
> > >   template <typename _InputIterator, typename _ForwardIterator,
> > >           typename _Allocator>
> > >--
> > >2.34.1
> > >
> >
> >
> >

Hi,

On 10/12/2024 10:56, Jonathan Wakely wrote:
> We need this to depend on is_trivially_copyable too, so we can use memcpy.
> 
> I'm testing a fix now to fix bootstrap.

There's a broader question I think, which is how much we want to "bend" 
the language rules.

[basic] isn't really super-explicit at giving us the rights to bit-blast 
bytes into uninitialized storage, and pretend there are objects there. 
There are a couple of provisions that deal with trivially copyable types:

https://eel.is/c++draft/basic.types.general#2
https://eel.is/c++draft/basic.types.general#3

which guarantee that one can memcpy the representation of an live object 
onto another live object, possibly itself. However, they don't 
necessarily talk about creating/starting the lifetime of new objects 
elsewhere.

That could be achieved through implicit-lifetime, but trivially copyable 
doesn't imply implicit-lifetime, AFAICS; consider the corner case of a 
type with deleted constructors but only trivial assignments and 
destruction, which is TC but not IL. This is why I think the facilities 
in P2590R2 talk about "trivially-copyable implicit-lifetime" types.

Maybe however the relocate functions are only called on types that we 
know are move constructible (e.g. from std::vector) so if they're TC we 
also know they're IL?

(The only thing missing is possibly a call to an abstract machine "magic 
function", such as start_lifetime_as?)

Explicitly checking for a trivial move constructor + trivial destructor 
(that is, the code after my patch) implies that the type is IL, but that 
doesn't really gives us the rights to assume that the object in the new 
storage have the same values than the old ones, as that's only 
guaranteed for TC types.

In short it looks like a bit of a lose-lose situation. I hope P3279 will 
bring clarity in the area.


Then, if the problem is just suppressing the warning, this does it:

> diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h
> index 916288352d7..374f30ee645 100644
> --- a/libstdc++-v3/include/bits/stl_uninitialized.h
> +++ b/libstdc++-v3/include/bits/stl_uninitialized.h
> @@ -1294,7 +1294,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>               return __out.base();
>             }
>  #endif
> -         __builtin_memcpy(__result, __first, __count * sizeof(_Tp));
> +         // Cast to void* in order to suppress -Wclass-memaccess.
> +         __builtin_memcpy(static_cast<void *>(__result), static_cast<const void *>(__first), __count * sizeof(_Tp));
>         }
>        return __result + __count;
>      }

It's the trick we use in Qt; or similarly just suppressing the warning 
via #pragmas, however I'm not sure if Clang likes that.

What do you think?

Thank you,

On Tue, 10 Dec 2024 at 10:48, Giuseppe D'Angelo
<giuseppe.dangelo@kdab.com> wrote:
>
> Hi,
>
> On 10/12/2024 10:56, Jonathan Wakely wrote:
> > We need this to depend on is_trivially_copyable too, so we can use memcpy.
> >
> > I'm testing a fix now to fix bootstrap.
>
> There's a broader question I think, which is how much we want to "bend"
> the language rules.

That's the subject of https://gcc.gnu.org/PR86023
And see also https://gcc.gnu.org/PR109976

> [basic] isn't really super-explicit at giving us the rights to bit-blast
> bytes into uninitialized storage, and pretend there are objects there.
> There are a couple of provisions that deal with trivially copyable types:
>
> https://eel.is/c++draft/basic.types.general#2
> https://eel.is/c++draft/basic.types.general#3
>
> which guarantee that one can memcpy the representation of an live object
> onto another live object, possibly itself. However, they don't
> necessarily talk about creating/starting the lifetime of new objects
> elsewhere.
>
> That could be achieved through implicit-lifetime, but trivially copyable
> doesn't imply implicit-lifetime, AFAICS; consider the corner case of a
> type with deleted constructors but only trivial assignments and
> destruction, which is TC but not IL. This is why I think the facilities
> in P2590R2 talk about "trivially-copyable implicit-lifetime" types.
>
> Maybe however the relocate functions are only called on types that we
> know are move constructible (e.g. from std::vector) so if they're TC we
> also know they're IL?

The relocate functions are called on nothrow-move types, and then we
dispatch to doing move+destroy or memcpy based on triviality.

>
> (The only thing missing is possibly a call to an abstract machine "magic
> function", such as start_lifetime_as?)
>
> Explicitly checking for a trivial move constructor + trivial destructor
> (that is, the code after my patch) implies that the type is IL, but that
> doesn't really gives us the rights to assume that the object in the new
> storage have the same values than the old ones, as that's only
> guaranteed for TC types.

That's partly why __is_bitwise_relocatable was intended as a
customization point. We can specialize it for std::pair<T,U> because
we know that if T and U are trivially copyable, then memcpy on
std::pair<T,U> gives the right values (even though technically
std::pair is never trivially copyable).

More pragmatically, without checking for trivially copyable or
disabling the warnings, there are bootstrap failures. So we do need a
quick fix.

>
> In short it looks like a bit of a lose-lose situation. I hope P3279 will
> bring clarity in the area.
>
>
> Then, if the problem is just suppressing the warning, this does it:
>
> > diff --git a/libstdc++-v3/include/bits/stl_uninitialized.h b/libstdc++-v3/include/bits/stl_uninitialized.h
> > index 916288352d7..374f30ee645 100644
> > --- a/libstdc++-v3/include/bits/stl_uninitialized.h
> > +++ b/libstdc++-v3/include/bits/stl_uninitialized.h
> > @@ -1294,7 +1294,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >               return __out.base();
> >             }
> >  #endif
> > -         __builtin_memcpy(__result, __first, __count * sizeof(_Tp));
> > +         // Cast to void* in order to suppress -Wclass-memaccess.
> > +         __builtin_memcpy(static_cast<void *>(__result), static_cast<const void *>(__first), __count * sizeof(_Tp));
> >         }
> >        return __result + __count;
> >      }
>
> It's the trick we use in Qt; or similarly just suppressing the warning
> via #pragmas, however I'm not sure if Clang likes that.
>
> What do you think?

I think this should be considered for GCC 16, rather than now. So I've
reverted the change to __is_bitwise_relocatable for now.