[Bug c++/112335] New: missed optimization on reset and assign unique_ptr

[Bug c++/112335] New: missed optimization on reset and assign unique_ptr

[federico at kircheis dot it]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

            Bug ID: 112335
           Summary: missed optimization on reset and assign unique_ptr
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: federico at kircheis dot it
  Target Milestone: ---

Given following snippet

----
#include <memory>

struct s{
    s();
    ~s() noexcept;
};



void bar1(std::unique_ptr<s>& ps1, std::unique_ptr<s>& ps2){
    ps1.reset();
    ps1 = std::move(ps2);
}

void bar2(std::unique_ptr<s>& ps1, std::unique_ptr<s>& ps2){
    ps1 = std::move(ps2);
}
----

If I am not mistaken, bar1 and bar2 should have exactly the same behavior, but
on https://godbolt.org/z/q4nKsPq7z it is possible to see that the generated
code differs.

[Bug c++/112335] missed optimization on reset and assign unique_ptr

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
  __old_p_6 = MEM[(struct s * &)ps1_2(D)];
  MEM[(struct s * &)ps1_2(D)] = 0B;
  if (__old_p_6 != 0B)
    goto <bb 3>; [53.47%]
  else
    goto <bb 4>; [46.53%]

  <bb 3> [local count: 574129752]:
  s::~s (__old_p_6);
  operator delete (__old_p_6, 1);

  <bb 4> [local count: 1073741824]:
  __p_4 = MEM[(struct s * &)ps2_3(D)];
  MEM[(struct s * &)ps2_3(D)] = 0B;
  __old_p_5 = MEM[(struct s * &)ps1_2(D)];

Well s::~s could touch the reference std::unique_ptr<s> (ps1).

So this could in theory be an invalid optimization.

I am not sure if that is the only issue here though.

[Bug c++/112335] missed optimization on reset and assign unique_ptr

[federico at kircheis dot it]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

--- Comment #2 from Federico Kircheis <federico at kircheis dot it> ---
> Well s::~s could touch the reference std::unique_ptr<s> (ps1).

In both cases, s::~s is called only once.

Also during the move-assignment no user-provided-code is involved (except the
destruction of ps1, and even case of a user-provided deleter, it is only called
if the pointer is not nullptr.

[Bug c++/112335] missed optimization on reset and assign unique_ptr

[federico at kircheis dot it]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

--- Comment #3 from Federico Kircheis <federico at kircheis dot it> ---
Or maybe I've misunderstood your comment.

Do you have a specific scenario in mind where the two snippets would exhibit
different behaviors?

[Bug c++/112335] missed optimization on reset and assign unique_ptr

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Federico Kircheis from comment #2)
> > Well s::~s could touch the reference std::unique_ptr<s> (ps1).
> 
> In both cases, s::~s is called only once.
> 
> Also during the move-assignment no user-provided-code is involved (except
> the destruction of ps1, and even case of a user-provided deleter, it is only
> called if the pointer is not nullptr.

NO. NO.


std::unique_ptr<s> t;

__thread bool tt;
inline s::~s()
{
  if (tt)
    return;
  tt = true;
  t.reset(new s);
  tt = false;
}

Is still a valid ~s as far as I can tell.
Yes it is odd but so what it is still valid.

Now will anyone write code like this, doubt it.

Also note clang does not optimize this with either libstdc++ or libc++.

[Bug c++/112335] missed optimization on reset and assign unique_ptr

[federico at kircheis dot it]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

--- Comment #5 from Federico Kircheis <federico at kircheis dot it> ---
Ok, the described case would be something like

----
std::unique_ptr<s> t;
__thread bool tt;
inline s::~s()
{
  if (tt)
    return;
  tt = true;
  t.reset(new s);
  tt = false;
}

std::unique_ptr<s> t2;
bar(t, t2);
----


The postcondition of p.reset() is that p == nullptr.

----
void reset(pointer p = pointer()) noexcept;
Preconditions: The expression get_deleter()(get()) is well-formed, has
well-defined behavior, and does not throw exceptions.
Effects: Assigns p to the stored pointer, and then if and only if the old value
of the stored pointer, old_p, was not equal to nullptr, calls
get_deleter()(old_p). [Note: The order of these operations is significant
because the call to get_deleter() may destroy *this. — end note]
Postconditions: get() == p. [Note: The postcondition does not hold if the call
to get_deleter() destroys *this since this->get() is no longer a valid
expression. — end note]
----

I do not think that the case you have in mind is supported by the standard, but
the postcondition does not hold with some deleters, so not sure if that could
be relevant here

[Bug c++/112335] missed optimization on reset and assign unique_ptr

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Federico Kircheis from comment #5)
> I do not think that the case you have in mind is supported by the standard,

It is. The standard is very clear about exactly which operations happen, and in
which order. Code can rely on that behaviour, because it's guaranteed by the
standard. Implementations are not allowed to deviate from that.

In your 'bar1' function when the ~s destructor runs it is guaranteed that it
will observe ps1.get() == nullptr, because that's the first thing that
ps1.reset() does.

In your 'bar2' function when the ~s destructor runs it's guaranteed that it
will observe ps1.get() == the original value of ps2, because that value gets
set before invoking the deleter.

Demo: https://godbolt.org/z/PWd96j4fz

That is clearly an observable difference, which is required by the standard,
and so your bar1 and bar2 are clearly not equivalent.

But a much simpler example where the difference is observable is when the two
arguments to bar1 and bar2 are the same object:
https://godbolt.org/z/z8fsTan8K
For bar1 you destroy any pointed-to object, then move-assign an empty pointer
to itself, which does nothing. So bar1(p, p,) is equivalent to p.reset();
For bar2 you release the pointer, then reset it back again. So bar2(p, p) is a
no-op.

The functions are not clearly equivalent.

> but the postcondition does not hold with some deleters, so not sure if that
> could be relevant here

I'm not sure what you mean here. The postcondition holds unless the deleter
*also* destroys the unique_ptr itself.

[Bug c++/112335] missed optimization on reset and assign unique_ptr

[federico at kircheis dot it]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

--- Comment #7 from Federico Kircheis <federico at kircheis dot it> ---
Thank you Jonathan, I've then misunderstood the example of Andrew, I thought he
was trying to create a scenario where after reset the pointer is not nullptr.

> Demo: https://godbolt.org/z/PWd96j4fz

Thank you for the example, but honestly, I think I am still missing something.

If you modify the main this way

----
int main()
{
    global = nullptr;
    std::unique_ptr<s> local1(new s);
    bar1(global, local1);
    global = nullptr;
    std::unique_ptr<s> local2(new s);
    bar1(global, local2);
}
----


or this way


----
int main()
{
    global = nullptr;
    std::unique_ptr<s> local1(new s);
    bar2(global, local1);
    global = nullptr;
    std::unique_ptr<s> local2(new s);
    bar2(global, local2);
}
----

the output is the same.
First time it prints 0, the second time a non-0 address.

> But a much simpler example where the difference is observable is when the two arguments to bar1 and bar2 are the same object:
> https://godbolt.org/z/z8fsTan8K

And this is absolutely correct, did not think of it.

> I'm not sure what you mean here. The postcondition holds unless the deleter *also* destroys the unique_ptr itself.

As written, I was not sure if this plays a role; this is what the standard says
(and I#ve misunderstood the example of Andrew).
A similar effect can be observed in the following example

----
#include <memory>

using s = int; // for simplicity

void bar1(std::unique_ptr<s>& ps1, std::unique_ptr<s>& ps2){
    ps1.reset();
    ps1 = std::move(ps2);
}
void bar2(std::unique_ptr<s>& ps1, std::unique_ptr<s>& ps2){
    ps1 = std::move(ps2);
}

void bar3(std::unique_ptr<s>& ps1, std::unique_ptr<s>& ps2){
    ps1.reset();
    ps1.reset();
    ps1 = std::move(ps2);
}
----

While you are right that bar1 and bar2 have different behavior if they point to
the same object, bar1 and bar3 are fundamentally the same function, even in
case of aliasing.
Unless the post-condition of reset is not hold; otherwise the second
"ps1.reset()" could be completely removed, as ps1 is nullptr.

[Bug c++/112335] missed optimization on reset and assign unique_ptr

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Federico Kircheis from comment #7)
> > Demo: https://godbolt.org/z/PWd96j4fz
> 
> Thank you for the example, but honestly, I think I am still missing
> something.
> 
> If you modify the main this way
> 
> ----
> int main()
> {
>     global = nullptr;
>     std::unique_ptr<s> local1(new s);
>     bar1(global, local1);
>     global = nullptr;
>     std::unique_ptr<s> local2(new s);
>     bar1(global, local2);
> }
> ----
> 
> 
> or this way
> 
> 
> ----
> int main()
> {
>     global = nullptr;
>     std::unique_ptr<s> local1(new s);
>     bar2(global, local1);
>     global = nullptr;
>     std::unique_ptr<s> local2(new s);
>     bar2(global, local2);
> }
> ----
> 
> the output is the same.
> First time it prints 0, the second time a non-0 address.

You're right, my example failed to show the difference I was trying to show,
which is that the value of ps1.get() observed by the destructor is different.
To show that, global.get() must be non-null initially, but it was always nul in
my example (the addresses being printed out where when the destructors ran in
main, not in bar1 and bar2).

Try this one: https://godbolt.org/z/KEbnPxEdq

This shows that in bar1 the value of ps1.get() is null when the destructor
runs. In bar2 the value of ps2.get() is not null when the destructor runs.

> 
> > But a much simpler example where the difference is observable is when the two arguments to bar1 and bar2 are the same object:
> > https://godbolt.org/z/z8fsTan8K
> 
> And this is absolutely correct, did not think of it.
> 
> > I'm not sure what you mean here. The postcondition holds unless the deleter *also* destroys the unique_ptr itself.
> 
> As written, I was not sure if this plays a role; this is what the standard
> says (and I#ve misunderstood the example of Andrew).
> A similar effect can be observed in the following example
> 
> ----
> #include <memory>
> 
> using s = int; // for simplicity
> 
> void bar1(std::unique_ptr<s>& ps1, std::unique_ptr<s>& ps2){
>     ps1.reset();
>     ps1 = std::move(ps2);
> }
> void bar2(std::unique_ptr<s>& ps1, std::unique_ptr<s>& ps2){
>     ps1 = std::move(ps2);
> }
> 
> void bar3(std::unique_ptr<s>& ps1, std::unique_ptr<s>& ps2){
>     ps1.reset();
>     ps1.reset();
>     ps1 = std::move(ps2);
> }
> ----
> 
> While you are right that bar1 and bar2 have different behavior if they point
> to the same object, bar1 and bar3 are fundamentally the same function, even
> in case of aliasing.
> Unless the post-condition of reset is not hold; otherwise the second
> "ps1.reset()" could be completely removed, as ps1 is nullptr.

The postcondition has to hold, by definition, that's what it means. It only
doesn't hold if the unique_ptr object itself ceases to exist, and that doesn't
happen with std::default_delete. It's irrelevant whether it happens with some
other deleters, because the code you're compiling doesn't use those other
deleters (if it did, it would be compiled differently).

This example is more interesting, bar1 does:

       test    r12, r12
        je      .L11
        mov     rdi, r12
        call    s::~s() [complete object destructor]
        mov     rdi, r12
        mov     esi, 1
        call    operator delete(void*, unsigned long)


But bar3 has extra instructions:

       test    r12, r12
        je      .L11
        mov     rdi, r12
        call    s::~s() [complete object destructor]
        mov     rdi, r12
        mov     esi, 1
        call    operator delete(void*, unsigned long)
        mov     r12, QWORD PTR [rbx]
        mov     QWORD PTR [rbx], 0
        test    r12, r12
        je      .L11
        mov     rdi, r12
        call    s::~s() [complete object destructor]
        mov     esi, 1
        mov     rdi, r12
        call    operator delete(void*, unsigned long)

I think the reason the redundant ps1.reset() isn't removed is that the compiler
doesn't know what operator delete(void*, size_t) does. In theory, the user
could replace that function with one which modifies a global unique_ptr, and
ps1 could alias that global.

https://godbolt.org/z/WGPTesEb3 shows that bar3 is not the same as bar1,
because it runs an additional destructor if operator delete() plays silly
games.

If we had PR 110137 then I think the redundant ps1.reset() would be optimized
away. Without it, the compiler assumes operator delete() might be silly.

Luckily, that bar3 code seems unlikely to be written. The second ps1.reset()
can obviously be removed by the programmer.

[Bug c++/112335] missed optimization on reset and assign unique_ptr

[federico at kircheis dot it]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

--- Comment #9 from Federico Kircheis <federico at kircheis dot it> ---
Great, thank you for the clarifications, your redacted example makes now sense.


> https://godbolt.org/z/WGPTesEb3 shows that bar3 is not the same as bar1, because it runs an additional destructor if operator delete() plays silly games.

And this is what I meant before that such program is not supported by the
standard

----
void bar3(std::unique_ptr<s>& ps1, std::unique_ptr<s>& ps2){
    ps1.reset();
    ps1.reset();
    ps1 = std::move(ps2);
}
---

is equivalent to

----
void bar3(std::unique_ptr<s>& ps1, std::unique_ptr<s>& ps2){
    ps1.reset();
    assert(ps1 == nullptr);
    ps1.reset();
    assert(ps1 == nullptr);
    ps1 = std::move(ps2);
}
----

which is not.

And the same holds if "if (!global) global.reset(new s);" is moved into "~s()",
and removed from "operator delete".

If the users changes delete in such a way, at this point we have UB, as the
postcondition of unique_ptr::reset does not hold (the example given by Andrew)

Yes, the second ps1.reset(); is trivial to remove.

[Bug c++/112335] missed optimization on reset and assign unique_ptr

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112335

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Ah, now I understand what you've been saying about the postcondition.

Yes, but the compiler doesn't know the postcondition, it's just words in the
standard, so not visible to the optimization passes.

It might be possible to add some optimization hints to the std::unique_ptr
implementation to make the postcondition visible to the optimizer.