www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 9985] New: Postblit isn't called on local struct return

reply d-bugmail puremagic.com writes:
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: -------
Apr 24 2013
next sibling parent d-bugmail puremagic.com writes:
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: -------
Apr 24 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Apr 24 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Apr 24 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Apr 24 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Apr 25 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Apr 25 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Apr 25 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Apr 25 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
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: -------
Apr 25 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9985



--- Comment #10 from monarchdodra gmail.com 2013-04-25 05:21:52 PDT ---
(In reply to comment #9)
 [SNIP]
Thanks. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Apr 25 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9985



--- Comment #11 from github-bugzilla puremagic.com 2013-04-28 12:46:59 PDT ---
Commits pushed to master at https://github.com/D-Programming-Language/dmd

https://github.com/D-Programming-Language/dmd/commit/15912c73192ef85d8cfaceb457c1af19dd575640
fix Issue 9985 - Postblit isn't called on local struct return

Until 'ref' deduction finished, auto ref function is not an actual ref
function. So compiler should keep NRVO ability.

https://github.com/D-Programming-Language/dmd/commit/66f3122cd17c1584db723370de27cd47f025e373
Merge pull request #1933 from 9rnsr/fix9985

Issue 9985 - Postblit isn't called on local struct return

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Apr 28 2013
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9985


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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


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