www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 5667] New: "clear" does not call destructors on embedded structs

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=5667

           Summary: "clear" does not call destructors on embedded structs
           Product: D
           Version: D1 & D2
          Platform: Other
        OS/Version: Windows
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody puremagic.com
        ReportedBy: samukha voliacable.com


--- Comment #0 from Max Samukha <samukha voliacable.com> 2011-02-28 10:53:40
PST ---
import std.traits;
import std.conv;
import core.stdc.stdlib;

struct S
{
    static int dtorCalled; 
    ~this()
    {
        dtorCalled++;                
    }
}

class C
{
    S s;    
}

struct S2
{
    S s;
}

void main()
{
    // 1
    enum size = __traits(classInstanceSize, C);
    auto buf = malloc(size)[0..size];
    emplace!C(buf);
    clear(buf);
    assert(S.dtorCalled == 1);
    free(buf.ptr);

    // 2
    S2* s2 = cast(S2*)malloc(S2.sizeof);
    clear(*s2);
    assert(S.dtorCalled == 2);
    free(cast(void*)s2);
}

While a solution to case 2 can be hacked up, case 1 requires a correct
destructor (one that calls destructors on the embedded objects and desirably
the base class destructor) and a pointer to that destructor in the classinfo.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 28 2011
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=5667


Steven Schveighoffer <schveiguy yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |schveiguy yahoo.com
            Summary|"clear" does not call       |"clear" does not call
                   |destructors on embedded     |destructors on structs
                   |structs                     |embedded in structs


--- Comment #1 from Steven Schveighoffer <schveiguy yahoo.com> 2011-02-28
11:14:17 PST ---
Your first case is invalid.  Clear depends on the type system:

    auto buf = malloc(size)[0..size];
    emplace!C(buf);
    clear(buf); // here, typeof(buf) == void[], will not call the class version

This does work:

    auto buf = malloc(size)[0..size];
    auto c = emplace!C(buf);
    clear(c);

The second case, I agree it's a bug.  Even clearing a stack-based struct does
not call the embedded dtor:

    S2 s2;
    clear(s2);
    assert(dtorCalled == 1); // fails

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



--- Comment #2 from Max Samukha <samukha voliacable.com> 2011-02-28 11:23:27
PST ---
(In reply to comment #1)

 This does work:
 
     auto buf = malloc(size)[0..size];
     auto c = emplace!C(buf);
     clear(c);
 


Damn, of course. Sorry.

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



--- Comment #3 from Cristi Cobzarenco <cristi.cobzarenco gmail.com> 2011-07-06
10:42:33 PDT ---
I think I found a fix, but I'm not sure if it's correct as I don't know much
about the D runtime. In file object_.d, line 2600:

void clear(T)(ref T obj) if (is(T == struct))
{
-   static if (is(typeof(obj.__dtor())))
-   {
-       obj.__dtor();
-   }
+   typeid(T).destroy( &obj );

    auto buf = (cast(ubyte*) &obj)[0 .. T.sizeof];
    auto init = cast(ubyte[])typeid(T).init();
    if(init.ptr is null) // null ptr means initialize to 0s
        buf[] = 0;
    else
        buf[] = init[];
}

This fixes it for me. You need to modifiy import/object.di as well, because
object_.d doesn't get header'd into object.di, for some reason I don't know.

I might make a pull request, but I'm not 100% this is correct, I would like to
get some feedback.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 06 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=5667



--- Comment #4 from Steven Schveighoffer <schveiguy yahoo.com> 2011-07-07
04:35:29 PDT ---
The code for TypeInfo_Struct.destroy is in object_.d:

https://github.com/D-Programming-Language/druntime/blob/master/src/object_.d#L986

It appears that it's using quite a different method to destroy the type
(somewhat necessary since TypeInfo is runtime information).  I'm curious why
calling __dtor isn't the same...

That probably should be the focus of this bug instead of working around the
issue.

(In reply to comment #3)


 This fixes it for me. You need to modifiy import/object.di as well, because
 object_.d doesn't get header'd into object.di, for some reason I don't know.


object.di is hand-maintained to remove some private things that are not
necessary to import.  The less things defined in the .di, the less internal
implementation is exposed, and the less chance someone can exploit
implementation details that shouldn't be relied upon.  I believe it is the only
hand-maintained interface file in druntime.

For templates, however, there is no possibility of implementation hiding -- the
entire source must be available.  I wonder if the templates of object_.d could
be separated into another module so we don't have to modify two files...

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 07 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=5667



--- Comment #5 from Cristi Cobzarenco <cristi.cobzarenco gmail.com> 2011-07-19
06:28:06 PDT ---
*** Issue 6203 has been marked as a duplicate of this issue. ***

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 19 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=5667


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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |k.hara.pg gmail.com


--- Comment #6 from Kenji Hara <k.hara.pg gmail.com> 2011-07-27 12:02:38 PDT ---
This is similar issue of bug5661.

T.__dtor == ~this(). So a struct type may not have __dtor.

But TypeInfo_Struct.xdtor specifies internal destructor calling function.
It calls *all* destructors of members even if itself does not have user-defined
destructor.

I think using typeof(T).destroy is right way for fixing this bug.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 27 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=5667



--- Comment #7 from Steven Schveighoffer <schveiguy yahoo.com> 2011-07-27
12:35:13 PDT ---
Yes, clearly that is a good path.

But my question is, why __dtor not the same as destroy?  Is there any reason to
call ~this() without calling all the sub-dtors?

In my opinion, ~this() should implicitly contain the __dtors for all the
members at the end.

But yes, having clear use typeid().destroy will fix the original symptom.  If
we don't intend to fix the root cause, we should at least document that nobody
should ever use __dtor...

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 27 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=5667



--- Comment #8 from Kenji Hara <k.hara.pg gmail.com> 2011-07-27 13:02:34 PDT ---
(In reply to comment #7)

 Yes, clearly that is a good path.
 
 But my question is, why __dtor not the same as destroy?  Is there any reason to
 call ~this() without calling all the sub-dtors?


Basically the name "__dtor" is beginning with double underscores, so it is
internal name. At least calling it is not "Right D Way", I think.

(This point is different from C++ syntax: T t(); t.~T();)


 In my opinion, ~this() should implicitly contain the __dtors for all the
 members at the end.


https://github.com/D-Programming-Language/dmd/blob/master/src/clone.c#L496

Looking at dmd source, the internal function name for its purpose may be
"__fieldDtor" or "__aggrDtor", but we can't call them explicitly.


 But yes, having clear use typeid().destroy will fix the original symptom.  If
 we don't intend to fix the root cause, we should at least document that nobody
 should ever use __dtor...


Agreed. Today calling __dtor is obviously incorrect idiom.

----
extern(C) int printf(const(char*) fmt, ...);
struct X
{
  ~this(){ printf("X.~this()\n"); }
}
struct S
{
  X x;
  ~this(){ printf("S.~this()\n"); }
}
void main()
{
  S s;
  s.__dtor();
  printf("-\n");
}
----

Output:
----
S.~this()    // s.__dtor() only call S.~this(), never call s.x.~this()
-
S.~this()
X.~this()
----

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Jul 27 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=5667



--- Comment #9 from Cristi Cobzarenco <cristi.cobzarenco gmail.com> 2011-07-28
04:07:39 PDT ---
Created a pull request with the fix and a unittest.

https://github.com/D-Programming-Language/druntime/pull/45

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


Cristi Cobzarenco <cristi.cobzarenco gmail.com> changed:

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


--- Comment #10 from Cristi Cobzarenco <cristi.cobzarenco gmail.com> 2011-08-15
09:26:53 PDT ---
The pull request was merged.

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