digitalmars.D.bugs - [Issue 4092] New: broken memory management for COM objects derived from IUnknown
- d-bugmail puremagic.com (68/68) Apr 14 2010 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (7/7) May 01 2010 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (12/12) Aug 04 2010 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (11/11) Feb 07 2011 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (33/33) Feb 08 2011 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (32/32) Feb 15 2011 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (47/50) Sep 27 2013 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (7/8) Sep 27 2013 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (8/9) Sep 27 2013 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (12/15) Sep 27 2013 Yes, that would be better than a synchronized block.
- d-bugmail puremagic.com (7/8) Sep 27 2013 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (11/13) Sep 27 2013 Hmm, you seem to have this prototype in that file:
- d-bugmail puremagic.com (11/14) Sep 27 2013 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (10/13) Sep 27 2013 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (10/10) Sep 27 2013 http://d.puremagic.com/issues/show_bug.cgi?id=4092
- d-bugmail puremagic.com (8/9) Sep 28 2013 http://d.puremagic.com/issues/show_bug.cgi?id=4092
http://d.puremagic.com/issues/show_bug.cgi?id=4092 Summary: broken memory management for COM objects derived from IUnknown Product: D Version: unspecified Platform: Other OS/Version: Windows Status: NEW Severity: normal Priority: P2 Component: druntime AssignedTo: sean invisibleduck.org ReportedBy: r.sagitario gmx.de PDT --- Currently, instances of classes that derive from IUnknown are allocated from the C-heap (see lifetime.d, function _d_newclass), but are never released in the default implementation ComObject (see std.c.windows.com, ComObject.Release()), because invariants might still be called. In addition, ComObjects are not known to the garbage collector (which is completely useless in at least 99% of all cases), so you have to override ComObject's new to avoid a bad collection while executing the class constructor. I suggest allocating ComObjects from standard garbage collected objects, and let the default imlpementation add ranges to the GC when AddRef is called with reference count 0, and removing the range when Release is called: class ComObject : IUnknown { // [... QueryInterface ...] override ULONG AddRef() { LONG lRef = InterlockedIncrement(&count); if(lRef == 1) { uint sz = this.classinfo.init.length; GC.addRange(cast(void*) this, sz); } return lRef; } override ULONG Release() { LONG lRef = InterlockedDecrement(&count); if (lRef == 0) { GC.removeRange(cast(void*) this); } return cast(ULONG)lRef; } } Otherwise, the garbage collector is responsible of releasing memory. This also lets ComObject be handled like normal objects in D-Code (no need to call AddRef/Release). Here's the patch to lifetime.d: Index: lifetime.d =================================================================== --- lifetime.d (revision 282) +++ lifetime.d (working copy) -98,7 +98,7 void* p; debug(PRINTF) printf("_d_newclass(ci = %p, %s)\n", ci, cast(char *)ci.name); - if (ci.m_flags & 1) // if COM object + if (0 & ci.m_flags & 1) // if COM object { /* COM objects are not garbage collected, they are reference counted * using AddRef() and Release(). They get free'd by C's free() * function called by Release() when Release()'s reference count goes -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Apr 14 2010
http://d.puremagic.com/issues/show_bug.cgi?id=4092 PDT --- I just recently noticed, that AddRef and Release should use addRoot and removeRoot, not addRange and removeRange. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
May 01 2010
http://d.puremagic.com/issues/show_bug.cgi?id=4092 Trass3r <mrmocool gmx.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mrmocool gmx.de Then also std.windows.iunknown should be removed and pragma(lib, "uuid") added to std.c.windows.com as mentioned there: http://d.puremagic.com/issues/show_bug.cgi?id=4295 -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Aug 04 2010
http://d.puremagic.com/issues/show_bug.cgi?id=4092 Walter Bright <bugzilla digitalmars.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bugzilla digitalmars.com 22:12:48 PST --- I'm not comfortable with this change. COM objects should be refcounted, not gc'd. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Feb 07 2011
http://d.puremagic.com/issues/show_bug.cgi?id=4092 PST --- The problem with allocating COM objects from the C-heap is that they cannot be free'd inside Release() due to possible invariants being called after that. Here's the implementation of Release in std.c.windows.com: ULONG Release() { LONG lRef = InterlockedDecrement(&count); if (lRef == 0) { // free object // If we delete this object, then the postinvariant called upon // return from Release() will fail. // Just let the GC reap it. //delete this; return 0; } return cast(ULONG)lRef; } The comment even implies that the memory should be taken from the GC. Also, any object that has references into other memory blocks needs to add itself as a root to the GC, which can be very easily forgotten (e.g. if the references are just strings). As reported lately, the juno project (http://www.dsource.org/projects/juno/wiki, probably the largest project trying to embrace COM), works similar as proposed here. ( http://www.digitalmars.com/pnews/read.php?server=news.digitalmars.com&group=digitalmars.D&artnum=128956 ) Agreed, changing this might break some code, but probably most applications creating COM objects have overloaded new anyway. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Feb 08 2011
http://d.puremagic.com/issues/show_bug.cgi?id=4092 PST --- Overloading new seems to do the job without the patch in the runtime, so here is my current implementation of COM objects: extern (C) void* gc_malloc( size_t sz, uint ba = 0 ); class ComObject : IUnknown { new(uint size) { void* p = gc_malloc(size, 1); // BlkAttr.FINALIZE return p; } override HRESULT QueryInterface(in IID* riid, void** ppv) { ... } override ULONG AddRef() { LONG lRef = InterlockedIncrement(&count); if(lRef == 1) GC.addRoot(cast(void*) this); return lRef; } override ULONG Release() { LONG lRef = InterlockedDecrement(&count); if (lRef == 0) GC.removeRoot(cast(void*) this); return cast(ULONG)lRef; } LONG count = 0; // object reference count } -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Feb 15 2011
http://d.puremagic.com/issues/show_bug.cgi?id=4092 Andrej Mitrovic <andrej.mitrovich gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrej.mitrovich gmail.com 07:10:31 PDT ---new(uint size)Should likely be size_t.LONG lRef = InterlockedIncrement(&count); LONG lRef = InterlockedDecrement(&count);Can a a synchronized block be used instead? These functions are declared in core.sys.win*, but they're commented out in dsource's WinAPI, with these comments: ----- /+ //-------------------------------------- // These functions are problematic version(UseNtoSKernel) {}else { /* CAREFUL: These are exported from ntoskrnl.exe and declared in winddk.h as __fastcall functions, but are exported from kernel32.dll as __stdcall */ static if (_WIN32_WINNT >= 0x0501) { VOID InitializeSListHead(PSLIST_HEADER); } LONG InterlockedCompareExchange(LPLONG, LONG, LONG); // PVOID WINAPI InterlockedCompareExchangePointer(PVOID*, PVOID, PVOID); (PVOID)InterlockedCompareExchange((LPLONG)(d) (PVOID)InterlockedCompareExchange((LPLONG)(d), (LONG)(e), (LONG)(c)) LONG InterlockedDecrement(LPLONG); LONG InterlockedExchange(LPLONG, LONG); // PVOID WINAPI InterlockedExchangePointer(PVOID*, PVOID); (PVOID)InterlockedExchange((LPLONG)((PVOID)InterlockedExchange((LPLONG)(t), (LONG)(v)) LONG InterlockedExchangeAdd(LPLONG, LONG); static if (_WIN32_WINNT >= 0x0501) { PSLIST_ENTRY InterlockedFlushSList(PSLIST_HEADER); } LONG InterlockedIncrement(LPLONG); static if (_WIN32_WINNT >= 0x0501) { PSLIST_ENTRY InterlockedPopEntrySList(PSLIST_HEADER); PSLIST_ENTRY InterlockedPushEntrySList(PSLIST_HEADER, PSLIST_ENTRY); } } // #endif // __USE_NTOSKRNL__ //-------------------------------------- +/ ----- -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
http://d.puremagic.com/issues/show_bug.cgi?id=4092 07:11:21 PDT ---Can a a synchronized block be used instead?Actually `atomicOp!"+="(count, 1)` could be used as well, no? -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
http://d.puremagic.com/issues/show_bug.cgi?id=4092 07:16:42 PDT ---void* p = gc_malloc(size, 1); // BlkAttr.FINALIZEAlso, this can actually now be: return GC.malloc(size, GC.BlkAttr.FINALIZE); -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
http://d.puremagic.com/issues/show_bug.cgi?id=4092 PDT ---Actually `atomicOp!"+="(count, 1)` could be used as well, no?Yes, that would be better than a synchronized block.Also, this can actually now be: return GC.malloc(size, GC.BlkAttr.FINALIZE);Yes. Unfortunately the introduction of precise scanning makes it impossible to use overloading new, because you don't get the actual TypeInfo for the allocation. I have switched to a template factory method since then: https://github.com/D-Programming-Language/visuald/blob/master/stdext/com.d#L28 The patch in druntime would make this obsolete. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
http://d.puremagic.com/issues/show_bug.cgi?id=4092 08:56:35 PDT ---https://github.com/D-Programming-Language/visuald/blob/master/stdext/com.d#L28Nice, this will be useful for me. Thanks. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
http://d.puremagic.com/issues/show_bug.cgi?id=4092 09:00:44 PDT ---https://github.com/D-Programming-Language/visuald/blob/master/stdext/com.d#L28Hmm, you seem to have this prototype in that file: extern(C) void* gc_malloc(size_t sz, uint ba = 0, const TypeInfo ti=null); However there is no such function which takes these parameters in druntime (in 2.064 anyway), the function takes 2 parameters, not 3. I suspect this links fine but would probably do something nasty like corrupt the stack. I suspect you've prototyped it to avoid an import into core.memory? -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
http://d.puremagic.com/issues/show_bug.cgi?id=4092 PDT ---Hmm, you seem to have this prototype in that file: extern(C) void* gc_malloc(size_t sz, uint ba = 0, const TypeInfo ti=null);This is the prototype for the precise GC, but it should do no harm elsewhere because the additional parameter in an extern(C) function is ignored by the called function and is removed from the stack by the callee. If you want to use the methods with the current GC and the respective imports, just leave out the third argument. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
http://d.puremagic.com/issues/show_bug.cgi?id=4092 13:33:33 PDT ---This is the prototype for the precise GC, but it should do no harm elsewhere because the additional parameter in an extern(C) function is ignored by the called function and is removed from the stack by the callee.Ah ok, thanks. Just one other thing, it's often mentioned that the reference count of a COM object should always start at 1, but your ComObject class starts with 0. Maybe this isn't an issue because it's incremented in the QueryInterface() call? -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
http://d.puremagic.com/issues/show_bug.cgi?id=4092 PDT --- The reference count might be slightly different than what's used in a language without GC, but I think starting at 0 is appropriate here. The object pointer can be used by the D program without any reference counting, it is only used to count (external) references to COM interfaces to avoid premature collection by keeping a root to the object in the GC. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
http://d.puremagic.com/issues/show_bug.cgi?id=4092 04:20:28 PDT ---The patch in druntime would make this obsolete.This issue really does need to be resolved, e.g. the newCom function can't be used for COM classes with private constructors. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 28 2013