| Message ID | alpine.DEB.2.02.2006040024400.2933614@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
| Series | std::optional defaut constructor | expand |
On Thu, 4 Jun 2020 at 01:52, Marc Glisse <marc.glisse@inria.fr> wrote: > > Hello, > > is there any drawback to the attached patch? It changes the code generated for I don't get it. The noexceptness of the defaulted default constructor should be a computation of the noexceptness of the subobjects, and that should boil down to whether optional's storage is noexcept-default-constructible. And that thing has a noexcept default constructor. Why does this patch change anything?
On Thu, 4 Jun 2020, Ville Voutilainen wrote: > On Thu, 4 Jun 2020 at 01:52, Marc Glisse <marc.glisse@inria.fr> wrote: >> >> Hello, >> >> is there any drawback to the attached patch? It changes the code generated for > > I don't get it. The noexceptness of the defaulted default constructor > should be a computation > of the noexceptness of the subobjects, and that should boil down to > whether optional's storage > is noexcept-default-constructible. And that thing has a noexcept > default constructor. Why > does this patch change anything? "noexcept" is a red herring, what matters is defaulted vs user-provided. In one case, we end up zero-initializing the whole buffer, and not in the other.
On Thu, 4 Jun 2020 at 02:13, Marc Glisse <marc.glisse@inria.fr> wrote: > > On Thu, 4 Jun 2020, Ville Voutilainen wrote: > > > On Thu, 4 Jun 2020 at 01:52, Marc Glisse <marc.glisse@inria.fr> wrote: > >> > >> Hello, > >> > >> is there any drawback to the attached patch? It changes the code generated for > > > > I don't get it. The noexceptness of the defaulted default constructor > > should be a computation > > of the noexceptness of the subobjects, and that should boil down to > > whether optional's storage > > is noexcept-default-constructible. And that thing has a noexcept > > default constructor. Why > > does this patch change anything? > > "noexcept" is a red herring, what matters is defaulted vs user-provided. > In one case, we end up zero-initializing the whole buffer, and not in the > other. Yes, I just came to that conclusion. This is value-init, so the language manages to zero-init the whole-object, but with the change, it just calls a user-provided constructor. That'll then merely boil down to value-initializing just the _Empty part of the _Storage in _Optional_payload_base. We are in http://eel.is/c++draft/dcl.init#8.1.2. The change takes us to http://eel.is/c++draft/dcl.init#8.1.1.
On Thu, 4 Jun 2020 at 02:20, Ville Voutilainen <ville.voutilainen@gmail.com> wrote: > > On Thu, 4 Jun 2020 at 02:13, Marc Glisse <marc.glisse@inria.fr> wrote: > > > > On Thu, 4 Jun 2020, Ville Voutilainen wrote: > > > > > On Thu, 4 Jun 2020 at 01:52, Marc Glisse <marc.glisse@inria.fr> wrote: > > >> > > >> Hello, > > >> > > >> is there any drawback to the attached patch? It changes the code generated for > > > > > > I don't get it. The noexceptness of the defaulted default constructor > > > should be a computation > > > of the noexceptness of the subobjects, and that should boil down to > > > whether optional's storage > > > is noexcept-default-constructible. And that thing has a noexcept > > > default constructor. Why > > > does this patch change anything? > > > > "noexcept" is a red herring, what matters is defaulted vs user-provided. > > In one case, we end up zero-initializing the whole buffer, and not in the > > other. > > Yes, I just came to that conclusion. This is value-init, so the > language manages to zero-init the whole-object, > but with the change, it just calls a user-provided constructor. > That'll then merely boil down to value-initializing just the _Empty > part > of the _Storage in _Optional_payload_base. We are in > http://eel.is/c++draft/dcl.init#8.1.2. The change takes us > to http://eel.is/c++draft/dcl.init#8.1.1. Ha, and optional's default constructor isn't even specified to be defaulted.
On Thu, 4 Jun 2020 at 03:05, Ville Voutilainen <ville.voutilainen@gmail.com> wrote: > > > "noexcept" is a red herring, what matters is defaulted vs user-provided. > > > In one case, we end up zero-initializing the whole buffer, and not in the > > > other. > > > > Yes, I just came to that conclusion. This is value-init, so the > > language manages to zero-init the whole-object, > > but with the change, it just calls a user-provided constructor. > > That'll then merely boil down to value-initializing just the _Empty > > part > > of the _Storage in _Optional_payload_base. We are in > > http://eel.is/c++draft/dcl.init#8.1.2. The change takes us > > to http://eel.is/c++draft/dcl.init#8.1.1. > > Ha, and optional's default constructor isn't even specified to be defaulted. So the change is correct. Can we test the change somehow?
On Thu, 4 Jun 2020, Ville Voutilainen wrote: > On Thu, 4 Jun 2020 at 03:05, Ville Voutilainen > <ville.voutilainen@gmail.com> wrote: > >>>> "noexcept" is a red herring, what matters is defaulted vs user-provided. >>>> In one case, we end up zero-initializing the whole buffer, and not in the >>>> other. >>> >>> Yes, I just came to that conclusion. This is value-init, so the >>> language manages to zero-init the whole-object, >>> but with the change, it just calls a user-provided constructor. >>> That'll then merely boil down to value-initializing just the _Empty >>> part >>> of the _Storage in _Optional_payload_base. We are in >>> http://eel.is/c++draft/dcl.init#8.1.2. The change takes us >>> to http://eel.is/c++draft/dcl.init#8.1.1. >> >> Ha, and optional's default constructor isn't even specified to be defaulted. > > So the change is correct. Can we test the change somehow? It passes the testsuite, and libc++ has been doing it this way for years. What I feared was some regression where it would yield worse code in some cases, or lose some property (not guaranteed by the standard) like triviality (to the point of affecting the ABI?), but I couldn't see anything like that happening. (we still have PR86173 causing unnecessary memset in some cases)
On Thu, 4 Jun 2020 at 10:22, Marc Glisse <marc.glisse@inria.fr> wrote: > > So the change is correct. Can we test the change somehow? > > It passes the testsuite, and libc++ has been doing it this way for years. > What I feared was some regression where it would yield worse code in some > cases, or lose some property (not guaranteed by the standard) like > triviality (to the point of affecting the ABI?), but I couldn't see > anything like that happening. > > (we still have PR86173 causing unnecessary memset in some cases) Right, I was just wondering whether we can reasonably verify in a test that the whole shebang is not zeroed. That may need a tree-dump scan in the test, and probably should go into PR86173 anyway, so I'm not saying such a thing needs to be a part of this fix. I'm kindly suggesting to Jonathan that this should be OK, and backports too.
On Thu, 4 Jun 2020, Ville Voutilainen wrote: > Right, I was just wondering whether we can reasonably verify in a test > that the whole shebang is not zeroed. That may need a tree-dump scan in > the test, and probably should go into PR86173 anyway, so I'm not saying > such a thing needs to be a part of this fix. The optimized dumps changed with the patch: - <retval> = {}; + MEM[(struct optional *)&<retval>] ={v} {CLOBBER}; MEM[(union _Storage *)&<retval>] ={v} {CLOBBER}; + MEM[(struct _Optional_payload_base *)&<retval>]._M_engaged = 0; return <retval>; checking for the absence of "<retval> = {}", or the presence of _M_engaged, may be robust enough across platforms. It doesn't really guarantee that nothing writes to the buffer though. Maybe create a buffer, fill it with some non-zero values (-1?), then call placement new, and read some value in the middle of the buffer, possibly with some protection against optimizations? Ah, no, actual constructors are fine, it is only the inlined initialization that happens with the defaulted constructor that zeroes things.
On Thu, 4 Jun 2020 at 11:00, Marc Glisse <marc.glisse@inria.fr> wrote: > Maybe create a buffer, fill it with some non-zero values (-1?), then call > placement new, and read some value in the middle of the buffer, possibly > with some protection against optimizations? Ah, no, actual constructors > are fine, it is only the inlined initialization that happens with the > defaulted constructor that zeroes things. The zero-init is part of value-initialization of a class type with a defaulted default constructor, so value-initialization with placement new should indeed show us whether the target buffer is zeroed.
On Thu, 4 Jun 2020, Ville Voutilainen wrote: > On Thu, 4 Jun 2020 at 11:00, Marc Glisse <marc.glisse@inria.fr> wrote: >> Maybe create a buffer, fill it with some non-zero values (-1?), then call >> placement new, and read some value in the middle of the buffer, possibly >> with some protection against optimizations? Ah, no, actual constructors >> are fine, it is only the inlined initialization that happens with the >> defaulted constructor that zeroes things. > > The zero-init is part of value-initialization of a class type with a > defaulted default constructor, so value-initialization with placement > new should indeed show us whether the target buffer is zeroed. Ah, yes, I had forgotten the empty () at the end of the operator new line when testing. Now the patch makes this runtime test go from abort to success at -O0 (with optimizations, the memset is removed as dead code). I am still not sure we want this kind of test though. And I added launder more to quiet a warning than with confidence that it does the right thing. #include <optional> struct A { int a[1024]; }; typedef std::optional<A> O; int main(){ unsigned char t[sizeof(O)]; __builtin_memset(t, -1, sizeof(t)); new(t)O(); if(std::launder(t)[512] != (unsigned char)(-1)) __builtin_abort(); }
On Thu, 4 Jun 2020 at 11:53, Marc Glisse <marc.glisse@inria.fr> wrote: > > On Thu, 4 Jun 2020, Ville Voutilainen wrote: > > > On Thu, 4 Jun 2020 at 11:00, Marc Glisse <marc.glisse@inria.fr> wrote: > >> Maybe create a buffer, fill it with some non-zero values (-1?), then call > >> placement new, and read some value in the middle of the buffer, possibly > >> with some protection against optimizations? Ah, no, actual constructors > >> are fine, it is only the inlined initialization that happens with the > >> defaulted constructor that zeroes things. > > > > The zero-init is part of value-initialization of a class type with a > > defaulted default constructor, so value-initialization with placement > > new should indeed show us whether the target buffer is zeroed. > > Ah, yes, I had forgotten the empty () at the end of the operator new line > when testing. Now the patch makes this runtime test go from abort to > success at -O0 (with optimizations, the memset is removed as dead code). I > am still not sure we want this kind of test though. And I added launder > more to quiet a warning than with confidence that it does the right thing. > > #include <optional> > struct A { > int a[1024]; > }; > typedef std::optional<A> O; > int main(){ > unsigned char t[sizeof(O)]; > __builtin_memset(t, -1, sizeof(t)); > new(t)O(); > if(std::launder(t)[512] != (unsigned char)(-1)) __builtin_abort(); > } Yeah, I think the patch is OK with or without the test. As a side note, you don't need the launder if the check uses the pointer value returned by placement-new.
On Thu, Jun 4, 2020 at 11:34 AM Ville Voutilainen via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Thu, 4 Jun 2020 at 11:53, Marc Glisse <marc.glisse@inria.fr> wrote: > > > > On Thu, 4 Jun 2020, Ville Voutilainen wrote: > > > > > On Thu, 4 Jun 2020 at 11:00, Marc Glisse <marc.glisse@inria.fr> wrote: > > >> Maybe create a buffer, fill it with some non-zero values (-1?), then call > > >> placement new, and read some value in the middle of the buffer, possibly > > >> with some protection against optimizations? Ah, no, actual constructors > > >> are fine, it is only the inlined initialization that happens with the > > >> defaulted constructor that zeroes things. > > > > > > The zero-init is part of value-initialization of a class type with a > > > defaulted default constructor, so value-initialization with placement > > > new should indeed show us whether the target buffer is zeroed. > > > > Ah, yes, I had forgotten the empty () at the end of the operator new line > > when testing. Now the patch makes this runtime test go from abort to > > success at -O0 (with optimizations, the memset is removed as dead code). I > > am still not sure we want this kind of test though. And I added launder > > more to quiet a warning than with confidence that it does the right thing. > > > > #include <optional> > > struct A { > > int a[1024]; > > }; > > typedef std::optional<A> O; > > int main(){ > > unsigned char t[sizeof(O)]; > > __builtin_memset(t, -1, sizeof(t)); > > new(t)O(); > > if(std::launder(t)[512] != (unsigned char)(-1)) __builtin_abort(); > > } > > Yeah, I think the patch is OK with or without the test. As a side > note, you don't need the launder > if the check uses the pointer value returned by placement-new. Doesn't the placement new make the memory state of anything not explicitely initialized indeterminate? That is, isn't the testcase broken anyways since GCC can elide the memset when seeing the placement new? Thanks, Richard.
On Thu, 4 Jun 2020 at 14:41, Richard Biener <richard.guenther@gmail.com> wrote: > Doesn't the placement new make the memory state of anything > not explicitely initialized indeterminate? That is, isn't the > testcase broken anyways since GCC can elide the memset > when seeing the placement new? Hmm, yes it does, and the test is broken.
On Thu, 4 Jun 2020, Ville Voutilainen wrote: > On Thu, 4 Jun 2020 at 11:53, Marc Glisse <marc.glisse@inria.fr> wrote: >> >> On Thu, 4 Jun 2020, Ville Voutilainen wrote: >> >>> On Thu, 4 Jun 2020 at 11:00, Marc Glisse <marc.glisse@inria.fr> wrote: >>>> Maybe create a buffer, fill it with some non-zero values (-1?), then call >>>> placement new, and read some value in the middle of the buffer, possibly >>>> with some protection against optimizations? Ah, no, actual constructors >>>> are fine, it is only the inlined initialization that happens with the >>>> defaulted constructor that zeroes things. >>> >>> The zero-init is part of value-initialization of a class type with a >>> defaulted default constructor, so value-initialization with placement >>> new should indeed show us whether the target buffer is zeroed. >> >> Ah, yes, I had forgotten the empty () at the end of the operator new line >> when testing. Now the patch makes this runtime test go from abort to >> success at -O0 (with optimizations, the memset is removed as dead code). I >> am still not sure we want this kind of test though. And I added launder >> more to quiet a warning than with confidence that it does the right thing. >> >> #include <optional> >> struct A { >> int a[1024]; >> }; >> typedef std::optional<A> O; >> int main(){ >> unsigned char t[sizeof(O)]; >> __builtin_memset(t, -1, sizeof(t)); >> new(t)O(); >> if(std::launder(t)[512] != (unsigned char)(-1)) __builtin_abort(); >> } > > Yeah, I think the patch is OK with or without the test. As a side > note, you don't need the launder > if the check uses the pointer value returned by placement-new. Yes, -fno-lifetime-dse is a better way to quiet the warning if optimizations are enabled and documents why this test is unsafe. Here is a version closer to what could go in the testsuite, although I'd still rather not add it at this point. We'll see what Jonathan thinks. (I didn't test this exact version) // { dg-options "-std=gnu++17 -fno-lifetime-dse" } // { dg-do run { target c++17 } } #include <optional> #include <testsuite_hooks.h> struct A { int a[1024]; }; typedef std::optional<A> O; void test01() { unsigned char t[sizeof(O)]; __builtin_memset(t, -1, sizeof(t)); new (t) O(); VERIFY( t[512] == (unsigned char)(-1) ); } int main() { test01(); }
On Thu, 4 Jun 2020, Richard Biener wrote: > On Thu, Jun 4, 2020 at 11:34 AM Ville Voutilainen via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> On Thu, 4 Jun 2020 at 11:53, Marc Glisse <marc.glisse@inria.fr> wrote: >>> >>> On Thu, 4 Jun 2020, Ville Voutilainen wrote: >>> >>>> On Thu, 4 Jun 2020 at 11:00, Marc Glisse <marc.glisse@inria.fr> wrote: >>>>> Maybe create a buffer, fill it with some non-zero values (-1?), then call >>>>> placement new, and read some value in the middle of the buffer, possibly >>>>> with some protection against optimizations? Ah, no, actual constructors >>>>> are fine, it is only the inlined initialization that happens with the >>>>> defaulted constructor that zeroes things. >>>> >>>> The zero-init is part of value-initialization of a class type with a >>>> defaulted default constructor, so value-initialization with placement >>>> new should indeed show us whether the target buffer is zeroed. >>> >>> Ah, yes, I had forgotten the empty () at the end of the operator new line >>> when testing. Now the patch makes this runtime test go from abort to >>> success at -O0 (with optimizations, the memset is removed as dead code). I >>> am still not sure we want this kind of test though. And I added launder >>> more to quiet a warning than with confidence that it does the right thing. >>> >>> #include <optional> >>> struct A { >>> int a[1024]; >>> }; >>> typedef std::optional<A> O; >>> int main(){ >>> unsigned char t[sizeof(O)]; >>> __builtin_memset(t, -1, sizeof(t)); >>> new(t)O(); >>> if(std::launder(t)[512] != (unsigned char)(-1)) __builtin_abort(); >>> } >> >> Yeah, I think the patch is OK with or without the test. As a side >> note, you don't need the launder >> if the check uses the pointer value returned by placement-new. > > Doesn't the placement new make the memory state of anything > not explicitely initialized indeterminate? That is, isn't the > testcase broken anyways since GCC can elide the memset > when seeing the placement new? Ah, I was just replying to that in parallel. Yes it is broken, that's why I don't really like adding it. But -fno-lifetime-dse may be enough to make it work if we really want to.
On 04/06/20 13:41 +0200, Richard Biener via Libstdc++ wrote: >On Thu, Jun 4, 2020 at 11:34 AM Ville Voutilainen via Gcc-patches ><gcc-patches@gcc.gnu.org> wrote: >> >> On Thu, 4 Jun 2020 at 11:53, Marc Glisse <marc.glisse@inria.fr> wrote: >> > >> > On Thu, 4 Jun 2020, Ville Voutilainen wrote: >> > >> > > On Thu, 4 Jun 2020 at 11:00, Marc Glisse <marc.glisse@inria.fr> wrote: >> > >> Maybe create a buffer, fill it with some non-zero values (-1?), then call >> > >> placement new, and read some value in the middle of the buffer, possibly >> > >> with some protection against optimizations? Ah, no, actual constructors >> > >> are fine, it is only the inlined initialization that happens with the >> > >> defaulted constructor that zeroes things. >> > > >> > > The zero-init is part of value-initialization of a class type with a >> > > defaulted default constructor, so value-initialization with placement >> > > new should indeed show us whether the target buffer is zeroed. >> > >> > Ah, yes, I had forgotten the empty () at the end of the operator new line >> > when testing. Now the patch makes this runtime test go from abort to >> > success at -O0 (with optimizations, the memset is removed as dead code). I >> > am still not sure we want this kind of test though. And I added launder >> > more to quiet a warning than with confidence that it does the right thing. >> > >> > #include <optional> >> > struct A { >> > int a[1024]; >> > }; >> > typedef std::optional<A> O; >> > int main(){ >> > unsigned char t[sizeof(O)]; >> > __builtin_memset(t, -1, sizeof(t)); >> > new(t)O(); >> > if(std::launder(t)[512] != (unsigned char)(-1)) __builtin_abort(); >> > } >> >> Yeah, I think the patch is OK with or without the test. As a side >> note, you don't need the launder >> if the check uses the pointer value returned by placement-new. > >Doesn't the placement new make the memory state of anything >not explicitely initialized indeterminate? That is, isn't the >testcase broken anyways since GCC can elide the memset >when seeing the placement new? Yes. IIUC -fno-lifetime-dse means the constructor that the placement new invokes doesn't clobber the old contents of the memory, but it seems fragile to rely on that remaining true in the long term.
On 04/06/20 10:34 +0300, Ville Voutilainen via Libstdc++ wrote: >On Thu, 4 Jun 2020 at 10:22, Marc Glisse <marc.glisse@inria.fr> wrote: > >> > So the change is correct. Can we test the change somehow? >> >> It passes the testsuite, and libc++ has been doing it this way for years. >> What I feared was some regression where it would yield worse code in some >> cases, or lose some property (not guaranteed by the standard) like >> triviality (to the point of affecting the ABI?), but I couldn't see >> anything like that happening. >> >> (we still have PR86173 causing unnecessary memset in some cases) > >Right, I was just wondering whether we can reasonably verify in a test >that the whole >shebang is not zeroed. That may need a tree-dump scan in the test, and probably >should go into PR86173 anyway, so I'm not saying such a thing needs to be a part >of this fix. > >I'm kindly suggesting to Jonathan that this should be OK, and backports too. Yes, looks good to me. Thanks, Marc. OK for master and gcc-10. I could be persuaded that it should go on gcc-9 too, if anybody feels strongly. Let's not change this in gcc-8 though, it's not required for correctness and isn't a codegen regression, and if it does cause a problem we won't get a chance to fix it after the next gcc-8 release.
On 04/06/20 00:50 +0200, Marc Glisse wrote: >(I don't currently have a setup that would enable me to commit anything. I'll >try to fix it eventually, but likely not so soon) Ah, I missed this bit. I'll take care of it for you. If it's due to the Git conversion let me know if it's something I can help with off-list. >diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional >index 37c2ba7a025..e84ba28a806 100644 >--- a/libstdc++-v3/include/std/optional >+++ b/libstdc++-v3/include/std/optional >@@ -688,7 +688,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > public: > using value_type = _Tp; > >- constexpr optional() = default; >+ constexpr optional() noexcept { } > > constexpr optional(nullopt_t) noexcept { } >
On 04/06/20 14:46 +0100, Jonathan Wakely wrote: >On 04/06/20 00:50 +0200, Marc Glisse wrote: >>(I don't currently have a setup that would enable me to commit anything. I'll >>try to fix it eventually, but likely not so soon) > >Ah, I missed this bit. I'll take care of it for you. > >If it's due to the Git conversion let me know if it's something I can >help with off-list. > > >>diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional >>index 37c2ba7a025..e84ba28a806 100644 >>--- a/libstdc++-v3/include/std/optional >>+++ b/libstdc++-v3/include/std/optional >>@@ -688,7 +688,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >> public: >> using value_type = _Tp; >> >>- constexpr optional() = default; >>+ constexpr optional() noexcept { } >> >> constexpr optional(nullopt_t) noexcept { } >> Committed to master as r11-1552 bafd12cb22e83b7da8946873513a897e48e2900f
diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional index 37c2ba7a025..e84ba28a806 100644 --- a/libstdc++-v3/include/std/optional +++ b/libstdc++-v3/include/std/optional @@ -688,7 +688,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION public: using value_type = _Tp; - constexpr optional() = default; + constexpr optional() noexcept { } constexpr optional(nullopt_t) noexcept { }
Hello, is there any drawback to the attached patch? It changes the code generated for std::optional<std::array<int,1024>>f(){return{};} from movq $0, (%rdi) movq %rdi, %r8 leaq 8(%rdi), %rdi xorl %eax, %eax movq $0, 4084(%rdi) movq %r8, %rcx andq $-8, %rdi subq %rdi, %rcx addl $4100, %ecx shrl $3, %ecx rep stosq movq %r8, %rax or with different tuning subq $8, %rsp movl $4100, %edx xorl %esi, %esi call memset addq $8, %rsp to the much shorter movb $0, 4096(%rdi) movq %rdi, %rax i.e. the same as the nullopt constructor. The constructor was already non-trivial, so we don't lose that. It passes the testsuite without regression. I thought I remembered there was a reason not to do this... 2020-06-04 Marc Glisse <marc.glisse@inria.fr> * include/std/optional (optional()): Explicitly define it. (I don't currently have a setup that would enable me to commit anything. I'll try to fix it eventually, but likely not so soon)