Issue 21693 - extern(C++) class instance dtors are never called, breaking RAII
Summary: extern(C++) class instance dtors are never called, breaking RAII
Status: NEW
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: All All
: P2 major
Assignee: No Owner
URL:
Keywords: C++
Depends on:
Blocks:
 
Reported: 2021-03-08 23:42 UTC by thomas.bockman
Modified: 2022-12-17 10:38 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description thomas.bockman 2021-03-08 23:42:27 UTC
The program below never destroys its extern(C++) class instances, breaking RAII. Output since 2.066:

    Destroying stack D instance.
    Destroying heap D instance.

The destructor should definitely be called for the `scope` instance, as it would be for a struct on the stack. The heap instance's destructor should also be called, OR trying to allocate an instance of an extern(C++) class that has a destructor on the GC heap using `new` should be a compile-time error.

/////////////////////////////////////
extern(C++) class C {
    bool onStack;
    this(bool onStack) {
        this.onStack = onStack; }
    ~this() {
        writeln("Destroying ", onStack? "stack" : "heap", " C instance."); }
}
extern(D) class D {
    bool onStack;
    this(bool onStack) {
        this.onStack = onStack; }
    ~this() {
        writeln("Destroying ", onStack? "stack" : "heap", " D instance."); }
}

void main() {
    scope stackC = new C(true);
    auto heapC = new C(false);
    scope stackD = new D(true);
    auto heapD = new D(false);
}
/////////////////////////////////////

Related: https://issues.dlang.org/show_bug.cgi?id=21692
Comment 1 moonlightsentinel 2021-03-11 02:52:31 UTC
Potential solution for `scope` instances:
https://github.com/dlang/dmd/pull/12265
Comment 2 Dlang Bot 2021-03-11 12:14:43 UTC
@MoonlightSentinel updated dlang/dmd pull request #12265 "Issue 21693 - Lower scoped destruction of extern(C++) class to destroy" mentioning this issue:

- Issue 21693 - Lower scoped destruction of extern(C++) class to destroy
  
  Ensures proper RAII behaviour for stack allocated instances but doesn't
  affect the behaviour for heap allocated instances.
  
  The previous rewrite used `delete` which relies on `TypeInfo` and
  crashed at runtime. Using `object.destroy` circumvents this issue (and
  is also beneficial due to the deprecation of `delete`).
  
  `destroy` can be used instead of `delete` because the instances live on
  the heap and hence don't need to be deallocated.

https://github.com/dlang/dmd/pull/12265
Comment 3 kinke 2021-03-13 21:34:48 UTC
The GC could presumably handle C++ classes like structs, where the TypeInfo_Struct pointer is stored after the struct instance IIRC. For C++ classes, it could store the D TypeInfo_Class pointer after the class instance (but note that class instances have no tail padding, so some padding might be needed for an aligned TypeInfo pointer).
Comment 4 Dlang Bot 2021-03-15 06:34:28 UTC
dlang/dmd pull request #12265 "Issue 21693 - Lower scoped destruction of extern(C++) class to destroy/dtor call" was merged into stable:

- e28736c522d9c81acef9c66c1f02da4375f08207 by MoonlightSentinel:
  Issue 21693 - Lower scoped destruction of extern(C++) class to destroy
  
  Ensures proper RAII behaviour for stack allocated instances but doesn't
  affect the behaviour for heap allocated instances.
  
  The previous rewrite used `delete` which relies on `TypeInfo` and
  crashed at runtime. Using `object.destroy` circumvents this issue (and
  is also beneficial due to the deprecation of `delete`).
  
  `destroy` can be used instead of `delete` because the instances live on
  the heap and hence don't need to be deallocated.

- aa2147b836b129e835828a6e6faef363ed3255a8 by MoonlightSentinel:
  fixup! Issue 21693 - Lower scoped destruction of extern(C++) class to destroy

https://github.com/dlang/dmd/pull/12265
Comment 5 thomas.bockman 2021-03-15 23:35:58 UTC
(In reply to kinke from comment #3)
> The GC could presumably handle C++ classes like structs, where the
> TypeInfo_Struct pointer is stored after the struct instance IIRC. For C++
> classes, it could store the D TypeInfo_Class pointer after the class
> instance (but note that class instances have no tail padding, so some
> padding might be needed for an aligned TypeInfo pointer).

That would be fine. The maximum overhead would be less than (size_t.sizeof * 2), which is no big deal if it gets RAII working properly.

Is it necessary to store a TypeInfo pointer for every instance, though? Garbage collected instances would all be allocated on the D side, so I would think that the TypeInfo data could just be stored adjacent to D's copy of the vtable.
Comment 6 thomas.bockman 2021-03-15 23:41:35 UTC
I'm dropping the priority from "critical" to "major" now that destruction is performed for `scope` instances, since the GC doesn't strictly guarantee deterministic destruction even for D class instances.

Thanks, @moonlightsentinel!
Comment 7 Dlang Bot 2021-03-24 10:39:14 UTC
@puneet updated dlang/dmd pull request #12302 "Fix issue 19192 - [wrong-code] [crashes] Covariant method interface <…" mentioning this issue:

- Issue 21693 - Lower scoped destruction of extern(C++) class to destroy/dtor call (#12265)
  
  * Issue 21693 - Lower scoped destruction of extern(C++) class to destroy
  
  Ensures proper RAII behaviour for stack allocated instances but doesn't
  affect the behaviour for heap allocated instances.
  
  The previous rewrite used `delete` which relies on `TypeInfo` and
  crashed at runtime. Using `object.destroy` circumvents this issue (and
  is also beneficial due to the deprecation of `delete`).
  
  `destroy` can be used instead of `delete` because the instances live on
  the heap and hence don't need to be deallocated.

https://github.com/dlang/dmd/pull/12302
Comment 8 Dlang Bot 2021-04-08 22:54:18 UTC
@MoonlightSentinel updated dlang/dmd pull request #12402 "Fix 21229 - Accept construction of union member as initialization" mentioning this issue:

- Issue 21693 - Lower scoped destruction of extern(C++) class to destroy/dtor call (#12265)
  
  * Issue 21693 - Lower scoped destruction of extern(C++) class to destroy
  
  Ensures proper RAII behaviour for stack allocated instances but doesn't
  affect the behaviour for heap allocated instances.
  
  The previous rewrite used `delete` which relies on `TypeInfo` and
  crashed at runtime. Using `object.destroy` circumvents this issue (and
  is also beneficial due to the deprecation of `delete`).
  
  `destroy` can be used instead of `delete` because the instances live on
  the heap and hence don't need to be deallocated.

https://github.com/dlang/dmd/pull/12402
Comment 9 Dlang Bot 2021-04-09 08:07:29 UTC
@MoonlightSentinel created dlang/dmd pull request #12408 "Merge stable into master" mentioning this issue:

- Issue 21693 - Lower scoped destruction of extern(C++) class to destroy/dtor call (#12265)
  
  * Issue 21693 - Lower scoped destruction of extern(C++) class to destroy
  
  Ensures proper RAII behaviour for stack allocated instances but doesn't
  affect the behaviour for heap allocated instances.
  
  The previous rewrite used `delete` which relies on `TypeInfo` and
  crashed at runtime. Using `object.destroy` circumvents this issue (and
  is also beneficial due to the deprecation of `delete`).
  
  `destroy` can be used instead of `delete` because the instances live on
  the heap and hence don't need to be deallocated.

https://github.com/dlang/dmd/pull/12408
Comment 10 Dlang Bot 2021-04-09 10:48:58 UTC
dlang/dmd pull request #12408 "Merge stable into master" was merged into master:

- 939778e46731edc6c4f726dda7f82aaa209d012e by Florian:
  Issue 21693 - Lower scoped destruction of extern(C++) class to destroy/dtor call (#12265)
  
  * Issue 21693 - Lower scoped destruction of extern(C++) class to destroy
  
  Ensures proper RAII behaviour for stack allocated instances but doesn't
  affect the behaviour for heap allocated instances.
  
  The previous rewrite used `delete` which relies on `TypeInfo` and
  crashed at runtime. Using `object.destroy` circumvents this issue (and
  is also beneficial due to the deprecation of `delete`).
  
  `destroy` can be used instead of `delete` because the instances live on
  the heap and hence don't need to be deallocated.

https://github.com/dlang/dmd/pull/12408