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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------