http://d.puremagic.com/issues/show_bug.cgi?id=9985

           Summary: Postblit isn't called on local struct return
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: major
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: SebastianGraf@t-online.de


--- Comment #0 from Sebastian Graf <SebastianGraf@t-online.de> 2013-04-24 07:44:50 PDT ---
For this program: http://dpaste.dzfl.pl/d73575a1
I get

    made S at 18FC64, s.b == 18FC68
    got back S at 18FCF4, s.b == 18FC68

as output. Note that no "postblit" message was printed.
Patching s.b to point into the newly allocated struct in postblit is crucial
here, but it seems like the postblit constructor isn't called, nor is there any
attempt
to optimize away the temporary in `makeS()` even with -O.
Am I doing something wrong?

http://dpaste.dzfl.pl/cc460feb copies the struct and ivokes postblit just fine. Maybe this is a bug in RVO?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9985


monarchdodra@gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |monarchdodra@gmail.com


--- Comment #1 from monarchdodra@gmail.com 2013-04-24 13:13:57 PDT ---
This is not a bug, but a feature.

D has very efficient move semantics, in particular [N]RVO, because D stipulates that stack objects may be *moved* without calling postblits.

One of the corollaries to this is that internal pointers are pointers to self are not accepted as legal code in D, and (as you have noticed) breaks the program.

Note that your second program exhibits the same behavior: you merely added an extra copy wich triggers the postblit. You have an intermediary temporary.

Rule of thumb is that you usually don't need internal pointers anyways. If you absolutely can't do without it, construct your object into a specific place, and *then* connect the pointers.

I'll close this if the explanation is okay with you.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9985



--- Comment #2 from Sebastian Graf <SebastianGraf@t-online.de> 2013-04-24 14:30:39 PDT ---
I stumbled over this when using a C library and it got me plenty time to trace it back. Seems like after all I missed one of those tiny spec details.

Anyhow, I even had a look at the generated assembly on windows, with/without
optimization.
There seems to be no NRVO applied, since makeS() does 2 copies (rep movsd), one
to copy S.init into s and one to copy s into __retval at exit. I may be
mistaken, but isn't the point of NRVO to make &s == &__retval, thus only
needing to copy S.init into &__retval?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9985



--- Comment #3 from monarchdodra@gmail.com 2013-04-24 14:37:55 PDT ---
(In reply to comment #2)
> I stumbled over this when using a C library and it got me plenty time to trace it back. Seems like after all I missed one of those tiny spec details.
> 
> Anyhow, I even had a look at the generated assembly on windows, with/without
> optimization.
> There seems to be no NRVO applied, since makeS() does 2 copies (rep movsd), one
> to copy S.init into s and one to copy s into __retval at exit. I may be
> mistaken, but isn't the point of NRVO to make &s == &__retval, thus only
> needing to copy S.init into &__retval?

I could be mistaken, but the "&s == &__retval" would be the "C++ NRVO". Since D is allowed to move things, it just detects that, and does a postblit-less memcopy, which is relatively simple and cheap.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9985



--- Comment #4 from Sebastian Graf <SebastianGraf@t-online.de> 2013-04-24 14:49:37 PDT ---
(In reply to comment #3)
> 
> I could be mistaken, but the "&s == &__retval" would be the "C++ NRVO". Since D is allowed to move things, it just detects that, and does a postblit-less memcopy, which is relatively simple and cheap.

Yeah, I meant NRVO in a C++ sense. So D doesn't attempt that and instead relies
on copying the struct without side effects, thus eliding destructors and
postblit.
Still seems awkward to me, why not just leave out copying all along? Or is
memcpy really that fast? If not, is this a potential optimization for the
future?

Sorry to hijack this into a learning thread. Feel free to close it any time.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9985



--- Comment #5 from Kenji Hara <k.hara.pg@gmail.com> 2013-04-25 00:56:11 PDT ---
(In reply to comment #0)
> For this program: http://dpaste.dzfl.pl/d73575a1

Don't link to external web site. Instead please directly paste the code in comment, or attach code file.

// Code:
import std.stdio;

struct S
{
    ubyte* b;
    ubyte buf[128];

    this(this)
    {
        writeln("postblit");
    }
}

auto ref makeS()
{
    S s;
    s.b = s.buf;
    writeln("made S at ", cast(void*)&s, ", s.b == ", s.b);
    return s;
}

void main()
{
    S s = makeS();
    writeln("got back S at ", cast(void*)&s, ", s.b == ", s.b);
}

> I get
> 
>     made S at 18FC64, s.b == 18FC68
>     got back S at 18FCF4, s.b == 18FC68
> 
> as output. Note that no "postblit" message was printed.
> Patching s.b to point into the newly allocated struct in postblit is crucial
> here, but it seems like the postblit constructor isn't called, nor is there any
> attempt
> to optimize away the temporary in `makeS()` even with -O.
> Am I doing something wrong?
> 
> http://dpaste.dzfl.pl/cc460feb

// Code:
import std.stdio;

struct S
{
    ubyte* b;
    ubyte buf[128];

    this(this)
    {
        writeln("postblit");
    }
}

auto ref makeS()
{
    S s;
    s.b = s.buf;
    writeln("made S at ", cast(void*)&s, ", s.b == ", s.b);
    return s;
}

void main()
{
    S s = makeS();
    writeln("got back S at ", cast(void*)&s, ", s.b == ", s.b);
}

> copies the struct and ivokes postblit just fine.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9985



--- Comment #6 from Kenji Hara <k.hara.pg@gmail.com> 2013-04-25 01:04:51 PDT ---
(In reply to comment #0)
> Maybe this is a bug in RVO?

This is a compiler bug at the intersection of the deduction for `auto ref` and NRVO.

If change the code
    auto ref makeS()
to:
    auto makeS()
The code would print the result as follows.

    made S at 12FDA0, s.b == 12FDA4
    got back S at 12FDA0, s.b == 12FDA4    <-- NRVO applied!

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9985


Kenji Hara <k.hara.pg@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |performance, pull,
                   |                            |wrong-code


--- Comment #7 from Kenji Hara <k.hara.pg@gmail.com> 2013-04-25 02:03:17 PDT ---
https://github.com/D-Programming-Language/dmd/pull/1933

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9985



--- Comment #8 from monarchdodra@gmail.com 2013-04-25 03:37:48 PDT ---
(In reply to comment #6)
> (In reply to comment #0)
> > Maybe this is a bug in RVO?
> 
> This is a compiler bug at the intersection of the deduction for `auto ref` and NRVO.

Could you clarify the "This is a compiler bug"? Are you saying this is an actual bug according to spec, or just that a "missed optimization opportunity" ?

In particular, if I compile using "S" instead of "auto ref", then NRVO only triggers in release. However, in non release, postblit still doesn't get called.

This is the correct behavior, correct? In non-release, there is no NRVO, but no postblit either, so the code is wrong according to spec?

================

I also want to note that the "NRVO fix" does not actually fix the original code, as passing a temp by value doesn't postblit. This will still fail, even in release, even with NRVO:

//--------
void doIt(S9985 s)
{
    assert(S9985.ptr == &s); //Passed without postblit, fails here

}
void main()
{
    doIt(makeS9985());
}
//--------

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9985



--- Comment #9 from Kenji Hara <k.hara.pg@gmail.com> 2013-04-25 04:50:10 PDT ---
(In reply to comment #8)
> Could you clarify the "This is a compiler bug"? Are you saying this is an actual bug according to spec, or just that a "missed optimization opportunity" ?

I say "missed optimization opportunity".

> In particular, if I compile using "S" instead of "auto ref", then NRVO only triggers in release. However, in non release, postblit still doesn't get called.
> 
> This is the correct behavior, correct? In non-release, there is no NRVO, but no postblit either, so the code is wrong according to spec?

With git head dmd, `S makeS()` and `auto makeS()` do NRVO. This is expected.
But `auto ref makeS()` doesn't NRVO. This is unexpected and a bug.

> ================
> 
> I also want to note that the "NRVO fix" does not actually fix the original code, as passing a temp by value doesn't postblit. This will still fail, even in release, even with NRVO:
> 
> //--------
> void doIt(S9985 s)
> {
>     assert(S9985.ptr == &s); //Passed without postblit, fails here
> 
> }
> void main()
> {
>     doIt(makeS9985());
> }
> //--------

makeS9985 returns an rvalue, and doIt receives the rvalue with 'move'. There is
no copy, so postblit is not called.
And, compiler does not apply NRVO in this case. Non-ref parameter `s` in doIt
function always means that "the given argument is moved in the function". So &s
is always different from S9985.ptr.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------