D issues are now tracked on GitHub. This Bugzilla instance remains as a read-only archive.
Issue 5667 - [GSoC] "clear" does not call destructors on structs embedded in structs
Summary: [GSoC] "clear" does not call destructors on structs embedded in structs
Status: RESOLVED FIXED
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: Other Windows
: P2 normal
Assignee: No Owner
URL:
Keywords: patch, wrong-code
: 6203 (view as issue list)
Depends on:
Blocks:
 
Reported: 2011-02-28 10:53 UTC by Max Samukha
Modified: 2015-06-09 05:12 UTC (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Max Samukha 2011-02-28 10:53:40 UTC
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.
Comment 1 Steven Schveighoffer 2011-02-28 11:14:17 UTC
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
Comment 2 Max Samukha 2011-02-28 11:23:27 UTC
(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.
Comment 3 Cristi Cobzarenco 2011-07-06 10:42:33 UTC
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.
Comment 4 Steven Schveighoffer 2011-07-07 04:35:29 UTC
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...
Comment 5 Cristi Cobzarenco 2011-07-19 06:28:06 UTC
*** Issue 6203 has been marked as a duplicate of this issue. ***
Comment 6 Kenji Hara 2011-07-27 12:02:38 UTC
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.
Comment 7 Steven Schveighoffer 2011-07-27 12:35:13 UTC
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...
Comment 8 Kenji Hara 2011-07-27 13:02:34 UTC
(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()
----
Comment 9 Cristi Cobzarenco 2011-07-28 04:07:39 UTC
Created a pull request with the fix and a unittest.

https://github.com/D-Programming-Language/druntime/pull/45
Comment 10 Cristi Cobzarenco 2011-08-15 09:26:53 UTC
The pull request was merged.