public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
* std::optional defaut constructor
@ 2020-06-03 22:50 Marc Glisse
  2020-06-03 23:05 ` Ville Voutilainen
  2020-06-04 13:46 ` Jonathan Wakely
  0 siblings, 2 replies; 20+ messages in thread
From: Marc Glisse @ 2020-06-03 22:50 UTC (permalink / raw)
  To: libstdc++, gcc-patches

[-- Attachment #1: Type: TEXT/PLAIN, Size: 997 bytes --]

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)

-- 
Marc Glisse

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: TEXT/x-diff; name=optional.patch, Size: 447 bytes --]

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 { }
 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-03 22:50 std::optional defaut constructor Marc Glisse
@ 2020-06-03 23:05 ` Ville Voutilainen
  2020-06-03 23:12   ` Marc Glisse
  2020-06-04 13:46 ` Jonathan Wakely
  1 sibling, 1 reply; 20+ messages in thread
From: Ville Voutilainen @ 2020-06-03 23:05 UTC (permalink / raw)
  To: Marc Glisse; +Cc: libstdc++, gcc-patches List

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?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-03 23:05 ` Ville Voutilainen
@ 2020-06-03 23:12   ` Marc Glisse
  2020-06-03 23:20     ` Ville Voutilainen
  0 siblings, 1 reply; 20+ messages in thread
From: Marc Glisse @ 2020-06-03 23:12 UTC (permalink / raw)
  To: Ville Voutilainen; +Cc: libstdc++, gcc-patches List

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.

-- 
Marc Glisse

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-03 23:12   ` Marc Glisse
@ 2020-06-03 23:20     ` Ville Voutilainen
  2020-06-04  0:05       ` Ville Voutilainen
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Voutilainen @ 2020-06-03 23:20 UTC (permalink / raw)
  To: Marc Glisse; +Cc: libstdc++, gcc-patches List

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-03 23:20     ` Ville Voutilainen
@ 2020-06-04  0:05       ` Ville Voutilainen
  2020-06-04  0:10         ` Ville Voutilainen
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Voutilainen @ 2020-06-04  0:05 UTC (permalink / raw)
  To: Marc Glisse; +Cc: libstdc++, gcc-patches List

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04  0:05       ` Ville Voutilainen
@ 2020-06-04  0:10         ` Ville Voutilainen
  2020-06-04  7:21           ` Marc Glisse
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Voutilainen @ 2020-06-04  0:10 UTC (permalink / raw)
  To: Marc Glisse; +Cc: libstdc++, gcc-patches List

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?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04  0:10         ` Ville Voutilainen
@ 2020-06-04  7:21           ` Marc Glisse
  2020-06-04  7:34             ` Ville Voutilainen
  0 siblings, 1 reply; 20+ messages in thread
From: Marc Glisse @ 2020-06-04  7:21 UTC (permalink / raw)
  To: Ville Voutilainen; +Cc: libstdc++, gcc-patches List

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)

-- 
Marc Glisse

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04  7:21           ` Marc Glisse
@ 2020-06-04  7:34             ` Ville Voutilainen
  2020-06-04  8:00               ` Marc Glisse
  2020-06-04 13:41               ` Jonathan Wakely
  0 siblings, 2 replies; 20+ messages in thread
From: Ville Voutilainen @ 2020-06-04  7:34 UTC (permalink / raw)
  To: Marc Glisse; +Cc: libstdc++, gcc-patches List

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04  7:34             ` Ville Voutilainen
@ 2020-06-04  8:00               ` Marc Glisse
  2020-06-04  8:25                 ` Ville Voutilainen
  2020-06-04 13:41               ` Jonathan Wakely
  1 sibling, 1 reply; 20+ messages in thread
From: Marc Glisse @ 2020-06-04  8:00 UTC (permalink / raw)
  To: Ville Voutilainen; +Cc: libstdc++, gcc-patches List

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.

-- 
Marc Glisse

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04  8:00               ` Marc Glisse
@ 2020-06-04  8:25                 ` Ville Voutilainen
  2020-06-04  8:53                   ` Marc Glisse
  0 siblings, 1 reply; 20+ messages in thread
From: Ville Voutilainen @ 2020-06-04  8:25 UTC (permalink / raw)
  To: Marc Glisse; +Cc: libstdc++, gcc-patches List

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04  8:25                 ` Ville Voutilainen
@ 2020-06-04  8:53                   ` Marc Glisse
  2020-06-04  9:20                     ` Ville Voutilainen
  0 siblings, 1 reply; 20+ messages in thread
From: Marc Glisse @ 2020-06-04  8:53 UTC (permalink / raw)
  To: Ville Voutilainen; +Cc: libstdc++, gcc-patches List

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();
}

-- 
Marc Glisse

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04  8:53                   ` Marc Glisse
@ 2020-06-04  9:20                     ` Ville Voutilainen
  2020-06-04 11:41                       ` Richard Biener
  2020-06-04 11:44                       ` Marc Glisse
  0 siblings, 2 replies; 20+ messages in thread
From: Ville Voutilainen @ 2020-06-04  9:20 UTC (permalink / raw)
  To: Marc Glisse; +Cc: libstdc++, gcc-patches List

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04  9:20                     ` Ville Voutilainen
@ 2020-06-04 11:41                       ` Richard Biener
  2020-06-04 11:43                         ` Ville Voutilainen
                                           ` (2 more replies)
  2020-06-04 11:44                       ` Marc Glisse
  1 sibling, 3 replies; 20+ messages in thread
From: Richard Biener @ 2020-06-04 11:41 UTC (permalink / raw)
  To: Ville Voutilainen; +Cc: Marc Glisse, libstdc++, gcc-patches List

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04 11:41                       ` Richard Biener
@ 2020-06-04 11:43                         ` Ville Voutilainen
  2020-06-04 11:46                         ` Marc Glisse
  2020-06-04 13:38                         ` Jonathan Wakely
  2 siblings, 0 replies; 20+ messages in thread
From: Ville Voutilainen @ 2020-06-04 11:43 UTC (permalink / raw)
  To: Richard Biener; +Cc: Marc Glisse, libstdc++, gcc-patches List

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04  9:20                     ` Ville Voutilainen
  2020-06-04 11:41                       ` Richard Biener
@ 2020-06-04 11:44                       ` Marc Glisse
  1 sibling, 0 replies; 20+ messages in thread
From: Marc Glisse @ 2020-06-04 11:44 UTC (permalink / raw)
  To: Ville Voutilainen; +Cc: libstdc++, gcc-patches List

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();
}


-- 
Marc Glisse

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04 11:41                       ` Richard Biener
  2020-06-04 11:43                         ` Ville Voutilainen
@ 2020-06-04 11:46                         ` Marc Glisse
  2020-06-04 13:38                         ` Jonathan Wakely
  2 siblings, 0 replies; 20+ messages in thread
From: Marc Glisse @ 2020-06-04 11:46 UTC (permalink / raw)
  To: Richard Biener; +Cc: Ville Voutilainen, libstdc++, gcc-patches List

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.

-- 
Marc Glisse

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04 11:41                       ` Richard Biener
  2020-06-04 11:43                         ` Ville Voutilainen
  2020-06-04 11:46                         ` Marc Glisse
@ 2020-06-04 13:38                         ` Jonathan Wakely
  2 siblings, 0 replies; 20+ messages in thread
From: Jonathan Wakely @ 2020-06-04 13:38 UTC (permalink / raw)
  To: Richard Biener
  Cc: Ville Voutilainen, libstdc++, gcc-patches List, Marc Glisse

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.


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04  7:34             ` Ville Voutilainen
  2020-06-04  8:00               ` Marc Glisse
@ 2020-06-04 13:41               ` Jonathan Wakely
  1 sibling, 0 replies; 20+ messages in thread
From: Jonathan Wakely @ 2020-06-04 13:41 UTC (permalink / raw)
  To: Ville Voutilainen; +Cc: Marc Glisse, libstdc++, gcc-patches List

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.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-03 22:50 std::optional defaut constructor Marc Glisse
  2020-06-03 23:05 ` Ville Voutilainen
@ 2020-06-04 13:46 ` Jonathan Wakely
  2020-06-19 11:50   ` Jonathan Wakely
  1 sibling, 1 reply; 20+ messages in thread
From: Jonathan Wakely @ 2020-06-04 13:46 UTC (permalink / raw)
  To: Marc Glisse; +Cc: libstdc++, gcc-patches

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 { }
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: std::optional defaut constructor
  2020-06-04 13:46 ` Jonathan Wakely
@ 2020-06-19 11:50   ` Jonathan Wakely
  0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Wakely @ 2020-06-19 11:50 UTC (permalink / raw)
  To: Marc Glisse; +Cc: libstdc++, gcc-patches

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



^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-06-19 11:50 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-03 22:50 std::optional defaut constructor Marc Glisse
2020-06-03 23:05 ` Ville Voutilainen
2020-06-03 23:12   ` Marc Glisse
2020-06-03 23:20     ` Ville Voutilainen
2020-06-04  0:05       ` Ville Voutilainen
2020-06-04  0:10         ` Ville Voutilainen
2020-06-04  7:21           ` Marc Glisse
2020-06-04  7:34             ` Ville Voutilainen
2020-06-04  8:00               ` Marc Glisse
2020-06-04  8:25                 ` Ville Voutilainen
2020-06-04  8:53                   ` Marc Glisse
2020-06-04  9:20                     ` Ville Voutilainen
2020-06-04 11:41                       ` Richard Biener
2020-06-04 11:43                         ` Ville Voutilainen
2020-06-04 11:46                         ` Marc Glisse
2020-06-04 13:38                         ` Jonathan Wakely
2020-06-04 11:44                       ` Marc Glisse
2020-06-04 13:41               ` Jonathan Wakely
2020-06-04 13:46 ` Jonathan Wakely
2020-06-19 11:50   ` Jonathan Wakely

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).