www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - I hate class destructors with a burning passion

reply Mathias LANG <geod24 gmail.com> writes:
Over the past few months, more than half of the critical bug 
we've encountered which were not due to our own code are related 
to destructors. Specifically, destructors being invoked by the GC.

In case you need a refresher, allocating from a destructor that 
is called from the GC leads to an `InvalidMemoryOperationError`. 
Which is pretty bad, because you don't even know *WHERE* the 
allocation happen, because `InvalidMemoryOperation` [does not 
give you a stack 
trace](https://github.com/dlang/druntime/blob/5c1873c7d95510f7e922f49ab46d815203f25471/src/cor
/exception.d#L279). Why ? Because generating a stack trace allocates. This also
mean you can't debug *other* failures in destructors, such as when an `assert`
fails (see below).

(Stack trace allocating can be fixed BTW, but requires a breaking 
change, because the `interface` we use provides you with 
already-formatted `const(char)[]` instead of structured data, but 
that's another discussion).

When such an `InvalidMemoryOperation` happens on your computer 
and is easy to debug, great. But when it happens once every blue 
moon on the production server... Not so great.

So what are the critical bugs we've seen over the last 6 months ?



We use Vibe.d extensively in our server app, and Vibe.d tries to 
clean after itself.
I think it's a reasonable assumption that, if you have an 
associative array, you should be able to call `remove` on it from 
a destructor, provided you can ensure it hasn't been collected.
Except, before v2.095.0, you couldn't, because `remove` might 
have called `shrink`, which allocates. [This bug has been fixed 
now](https://issues.dlang.org/show_bug.cgi?id=21442).



I'm not talking about message formatting here, but pure `assert(a 
!= a)` allocating.
This bug has been known for [almost a 
decade](https://issues.dlang.org/show_bug.cgi?id=7349) and was 
finally fixed in v2.097.0 ([it wasn't that 
hard](https://github.com/dlang/druntime/pull/3476))


broken since 2.096.0

Why did we have an assertion failure in our destructor ?
Well turns out there was [an `assert` checking a 
magic](https://github.com/vibe-d/vibe-core/issues/283), and it 
was being triggered. Why ? Because of a [regression introduced in 
v2.096.0](https://issues.dlang.org/show_bug.cgi?id=21989).


This is the top 3 that we could track down. We currently have a 
random crash which seems to trigger when a C++ object is 
destructed by the GC (to be precise: the GC finalizes a `class` 
that holds an `std::vector`), but that might just be our 
bindings...
Jun 06
next sibling parent reply Imperatorn <johan_forsberg_86 hotmail.com> writes:
On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 Over the past few months, more than half of the critical bug 
 we've encountered which were not due to our own code are 
 related to destructors. Specifically, destructors being invoked 
 by the GC.

 [...]
What would be some sensible solutions to these issues?
Jun 06
parent reply Guillaume Piolat <first.last gmail.com> writes:
On Sunday, 6 June 2021 at 14:19:39 UTC, Imperatorn wrote:
 On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 Over the past few months, more than half of the critical bug 
 we've encountered which were not due to our own code are 
 related to destructors. Specifically, destructors being 
 invoked by the GC.

 [...]
What would be some sensible solutions to these issues?
Two first issues can be avoided by not letting the GC call class destructors, and call them deterministically instead.
Jun 06
next sibling parent reply Guillaume Piolat <first.last gmail.com> writes:
On Sunday, 6 June 2021 at 18:55:47 UTC, Guillaume Piolat wrote:
 On Sunday, 6 June 2021 at 14:19:39 UTC, Imperatorn wrote:
 On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 Over the past few months, more than half of the critical bug 
 we've encountered which were not due to our own code are 
 related to destructors. Specifically, destructors being 
 invoked by the GC.

 [...]
What would be some sensible solutions to these issues?
Two first issues can be avoided by not letting the GC call class destructors, and call them deterministically instead.
Also, you knew it was coming: http://p0nce.github.io/d-idioms/#GC-proof-resource-class is all you need and has been an existing solution for 6 years.
Jun 06
parent reply Ogi <ogion.art gmail.com> writes:
On Sunday, 6 June 2021 at 19:00:48 UTC, Guillaume Piolat wrote:
 Two first issues can be avoided by not letting the GC call 
 class destructors, and call them deterministically instead.
Also, you knew it was coming: http://p0nce.github.io/d-idioms/#GC-proof-resource-class is all you need and has been an existing solution for 6 years.
Conventions do not scale.
Jun 07
parent Guillaume Piolat <first.last gmail.com> writes:
On Monday, 7 June 2021 at 10:28:32 UTC, Ogi wrote:
 On Sunday, 6 June 2021 at 19:00:48 UTC, Guillaume Piolat wrote:
 Two first issues can be avoided by not letting the GC call 
 class destructors, and call them deterministically instead.
Also, you knew it was coming: http://p0nce.github.io/d-idioms/#GC-proof-resource-class is all you need and has been an existing solution for 6 years.
Conventions do not scale.
Ignore classic solutions, get classic problems.
Jun 07
prev sibling parent reply Imperatorn <johan_forsberg_86 hotmail.com> writes:
On Sunday, 6 June 2021 at 18:55:47 UTC, Guillaume Piolat wrote:
 On Sunday, 6 June 2021 at 14:19:39 UTC, Imperatorn wrote:
 On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 Over the past few months, more than half of the critical bug 
 we've encountered which were not due to our own code are 
 related to destructors. Specifically, destructors being 
 invoked by the GC.

 [...]
What would be some sensible solutions to these issues?
Two first issues can be avoided by not letting the GC call class destructors, and call them deterministically instead.
Do we have an issue or two for that? Sounds like an enhancement. Should/would it be configurable? Like -gcClassDtors=yes/no/auto kind of a thing or just kill it.
Jun 07
next sibling parent reply Mike Parker <aldacron gmail.com> writes:
On Monday, 7 June 2021 at 10:04:32 UTC, Imperatorn wrote:
 On Sunday, 6 June 2021 at 18:55:47 UTC, Guillaume Piolat wrote:

 Two first issues can be avoided by not letting the GC call 
 class destructors, and call them deterministically instead.
Do we have an issue or two for that? Sounds like an enhancement. Should/would it be configurable? Like -gcClassDtors=yes/no/auto kind of a thing or just kill it.
`GC.inFinalizer` was added specifically to address this issue. ```D if(GC.inFinalizer) { // no GC or deterministic stuff allowed here } else { // do whatever you want } ``` So if you need GC or deterministic stuff in a class destructor, you can invoke it manually and still let the GC worry about cleaning up the memory. The alternatives (adding a separate finalizer, or preventing the GC from calling destructors) are massively breaking changes, so I don't see that happening. Even had destruction and finalization been separate from the beginning, I'm sure people would have misused the finalizer anyway just as they did in older versions of Java.
Jun 07
parent Ola Fosheim =?UTF-8?B?R3LDuHN0YWQ=?= <ola.fosheim.grostad gmail.com> writes:
On Monday, 7 June 2021 at 10:51:02 UTC, Mike Parker wrote:
 The alternatives (adding a separate finalizer, or preventing 
 the GC from calling destructors) are massively breaking 
 changes, so I don't see that happening. Even had destruction 
 and finalization been separate from the beginning, I'm sure 
 people would have misused the finalizer anyway just as they did 
 in older versions of Java.
Breaking changes for memory management are eventually needed, but they should not come one after another. It has to be designed as a whole. So, if you want finalization, you need to start by making strongly typed unions and enable precise collection.
Jun 07
prev sibling parent reply Guillaume Piolat <first.last gmail.com> writes:
On Monday, 7 June 2021 at 10:04:32 UTC, Imperatorn wrote:
 Do we have an issue or two for that? Sounds like an 
 enhancement. Should/would it be configurable? Like 
 -gcClassDtors=yes/no/auto kind of a thing or just kill it.
That the GC would not call class destructors have been proposed by Alexandrescu in this very community, which kindly rejected the change, years ago. This can be done if everyone first jump into the deterministic destruction wagon, since programs are currently accidentally correct. But you'll find that people do not agree on the problem. So, in the absence of consensus, idioms like the one I posted are what we have right now.
Jun 07
parent Guillaume Piolat <first.last gmail.com> writes:
On Monday, 7 June 2021 at 12:17:18 UTC, Guillaume Piolat wrote:
 That the GC would not call class destructors have been proposed 
 by Alexandrescu in this very community, which kindly rejected 
 the change, years ago.
See_also: https://forum.dlang.org/post/ljrm0d$28vf$1 digitalmars.com
Jun 07
prev sibling next sibling parent reply apz28 <home home.com> writes:
On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 Over the past few months, more than half of the critical bug 
 we've encountered which were not due to our own code are 
 related to destructors. Specifically, destructors being invoked 
 by the GC.
Is there anyway to build runtime with pre-allocated buffer on startup and use it to return error-text/stack-trace? This is how Delphi does for out of memory exception & when using debug build runtime
Jun 06
parent reply mw <mingwu gmail.com> writes:
On Sunday, 6 June 2021 at 15:01:31 UTC, apz28 wrote:
 On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 Over the past few months, more than half of the critical bug 
 we've encountered which were not due to our own code are 
 related to destructors. Specifically, destructors being 
 invoked by the GC.
Is there anyway to build runtime with pre-allocated buffer on startup and use it to return error-text/stack-trace? This is how Delphi does for out of memory exception & when using debug build runtime
That's a good idea, actually even a limited sized buffer is good enough, i.e the few top stack frames is mostly useful for the developers to identify the problem (even for stack overflow, e.g. recursion without termination condition). Mathias, hope this approach will take less work than you mentioned in the other post: https://forum.dlang.org/post/hyayanlrcxuyljgetfms forum.dlang.org
Jun 06
parent reply mw <mingwu gmail.com> writes:
On Sunday, 6 June 2021 at 17:42:40 UTC, mw wrote:
 On Sunday, 6 June 2021 at 15:01:31 UTC, apz28 wrote:
 On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
[...]
Is there anyway to build runtime with pre-allocated buffer on startup and use it to return error-text/stack-trace? This is how Delphi does for out of memory exception & when using debug build runtime
That's a good idea, actually even a limited sized buffer is
We can even add a compiler flag: let the developer specify the buffer size, e.g. in debug build: 1024 * 100, and 1024 or 0 in release build.
 good enough, i.e the few top stack frames is mostly useful for 
 the developers to identify the problem (even for stack 
 overflow, e.g. recursion without termination condition).

  Mathias, hope this approach will take less work than you 
 mentioned in the other post:

 https://forum.dlang.org/post/hyayanlrcxuyljgetfms forum.dlang.org
Jun 06
parent reply rikki cattermole <rikki cattermole.co.nz> writes:
On 07/06/2021 5:48 AM, mw wrote:
 We can even add a compiler flag: let the developer specify the buffer 
 size, e.g. in debug build: 1024 * 100, and 1024 or 0 in release build.
It could be allocated using the DRT argument handling allowing it to be set at runtime rather than just at compilation.
Jun 06
parent reply mw <mingwu gmail.com> writes:
On Sunday, 6 June 2021 at 18:24:45 UTC, rikki cattermole wrote:
 On 07/06/2021 5:48 AM, mw wrote:
 We can even add a compiler flag: let the developer specify the 
 buffer size, e.g. in debug build: 1024 * 100, and 1024 or 0 in 
 release build.
It could be allocated using the DRT argument handling allowing it to be set at runtime rather than just at compilation.
I've thought about it, but think it's better be controlled by the compiler: e.g. in release mode, commercial developers may not want the software end-user to peek into the trade secret by specifying a bigger buffer size flag at runtime, so s/he can see the stack frames the developers not intended to publish.
Jun 06
parent rikki cattermole <rikki cattermole.co.nz> writes:
On 07/06/2021 6:37 AM, mw wrote:
 I've thought about it, but think it's better be controlled by the 
 compiler: e.g. in release mode, commercial developers may not want the 
 software end-user to peek into the trade secret by specifying a bigger 
 buffer size flag at runtime, so s/he can see the stack frames the 
 developers not intended to publish.
If the information exists in the binary, it can always be acquired by those that want it in that scenario. I.e. by attaching a debugger.
Jun 06
prev sibling next sibling parent reply tsbockman <thomas.bockman gmail.com> writes:
On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 We currently have a random crash which seems to trigger when a
 C++ object is destructed by the GC (to be precise: the GC
 finalizes a `class` that holds an `std::vector`), but that
 might just be our bindings...
extern(C++) class support is critically incomplete/buggy right now, even for basic usage that doesn't involve templates or multiple inheritance. Are you aware of these bugs? Issue 21693 - extern(C++) class instance dtors are never called, breaking RAII: https://issues.dlang.org/show_bug.cgi?id=21693 Issue 21690 - Unable to dynamic cast extern(C++) classes (Dynamic casts of extern(C++) classes are silently lowered to unchecked static casts): https://issues.dlang.org/show_bug.cgi?id=2169
Jun 06
parent Mathias LANG <geod24 gmail.com> writes:
On Sunday, 6 June 2021 at 22:00:32 UTC, tsbockman wrote:
 On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 We currently have a random crash which seems to trigger when a
 C++ object is destructed by the GC (to be precise: the GC
 finalizes a `class` that holds an `std::vector`), but that
 might just be our bindings...
extern(C++) class support is critically incomplete/buggy right now, even for basic usage that doesn't involve templates or multiple inheritance. Are you aware of these bugs? Issue 21693 - extern(C++) class instance dtors are never called, breaking RAII: https://issues.dlang.org/show_bug.cgi?id=21693 Issue 21690 - Unable to dynamic cast extern(C++) classes (Dynamic casts of extern(C++) classes are silently lowered to unchecked static casts): https://issues.dlang.org/show_bug.cgi?id=2169
Thanks for the pointers. I checked and I don't think we're using any of those, luckily. The first one, because we don't allocate those objects standalone, we always store them in another object, so their dtors are called (hence the bug). We don't need dynamic cast (we have a few classes called from C++) so this is all fine for us. The major bug that blocked us for a long time was https://issues.dlang.org/show_bug.cgi?id=20235 which was fixed in v2.096.0.
Jun 09
prev sibling next sibling parent Jack <jckj33 gmail.com> writes:
On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 Over the past few months, more than half of the critical bug 
 we've encountered which were not due to our own code are 
 related to destructors. Specifically, destructors being invoked 
 by the GC.

 [...]
Should not we *not* using this? https://dlang.org/blog/2021/03/04/symphony-of-destruction-structs-classes-and-the-gc-part-one/
Jun 07
prev sibling next sibling parent reply Kagamin <spam here.lot> writes:
On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 [an `assert` checking a 
 magic](https://github.com/vibe-d/vibe-core/issues/283)
As I understand, this code unintentionally accesses thread local connection pool in multithreaded manner without synchronization?
Jun 08
parent Kagamin <spam here.lot> writes:
On Tuesday, 8 June 2021 at 08:11:38 UTC, Kagamin wrote:
 On Sunday, 6 June 2021 at 11:54:40 UTC, Mathias LANG wrote:
 [an `assert` checking a 
 magic](https://github.com/vibe-d/vibe-core/issues/283)
As I understand, this code unintentionally accesses thread local connection pool in multithreaded manner without synchronization?
https://github.com/vibe-d/vibe-http/blob/master/source/vibe/http/client.d#L216 and looks like on connection to 17th host the first pool is freed from the cache and later collected as garbage, so its lifetime is limited, but you touch it in destructor, which is UAF.
Jun 08
prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 6/6/2021 4:54 AM, Mathias LANG wrote:
 https://github.com/dlang/druntime/pull/3476/files
Frankly, the whole jazz with assert() went way off the rails. Assert should go directly to Jail, it should not pass Go or collect $200. All these layers it goes through is just madness. Allocating via the GC is in assert is just ridiculous, as the program is going to exit shortly anyway. There won't ever be a need to run a collection on it. Just malloc away and fuggeddabootet. These sorts of things are why I added the checkaction switch to bypass it and just do the simple, direct stuff. --- Throwing constructors is one of those things that makes a program very hard to reason about. I've debated with Andrei about requiring that constructors be nothrow. My advice is make them nothrow, i.e. design them so they cannot fail. There's not a single throwing constructor in DMD, and it's going to stay that way :-) --- A destructor should never try to remove a GC allocated member. The GC may have already removed it! As with constructors, design destructors so they cannot fail. You'll have a lot less heartache that way. --- This is not to argue that we don't need to fix the bugs. Thanks, Mathias, for the fixes you've created! It's appreciated.
Jun 08
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 6/8/21 10:37 PM, Walter Bright wrote:
 Throwing constructors is one of those things that makes a program very 
 hard to reason about. I've debated with Andrei about requiring that 
 constructors be nothrow. My advice is make them nothrow, i.e. design 
 them so they cannot fail. There's not a single throwing constructor in 
 DMD, and it's going to stay that way :-)
Except for FileMapping, and that's a Good Thing(tm): https://github.com/dlang/dmd/pull/12560/files?file-filters%5B%5D=.c&file-filters%5B%5D=.d&file-filters%5B%5D=.md#diff-0be8539d9165e1607c7b47964ebf59a507c9ab436c6c4350df36d1c61d111a50R74 I'm half joking - that cosntructor may terminate the application. Throwing constructors are an important part of achieving good encapsulation because they allow avoiding "invalid" states of objects altogether. In fairness, the fact that D has no user-definable default constructors weakens that argument (and is a weakness of the language itself that we'd do good to fix).
Jun 08
next sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Wed, Jun 09, 2021 at 12:58:49AM -0400, Andrei Alexandrescu via Digitalmars-d
wrote:
[...]
 In fairness, the fact that D has no user-definable default
 constructors weakens that argument (and is a weakness of the language
 itself that we'd do good to fix).
This only applies to structs. Classes do have user-definable default ctors. Nevertheless, the lack of user-definable default ctors in structs has been a frequent complaint. T -- Latin's a dead language, as dead as can be; it killed off all the Romans, and now it's killing me! -- Schoolboy
Jun 09
prev sibling parent Walter Bright <newshound2 digitalmars.com> writes:
On 6/8/2021 9:58 PM, Andrei Alexandrescu wrote:
 Throwing constructors are an important part of achieving good encapsulation 
 because they allow avoiding "invalid" states of objects altogether. In
fairness, 
 the fact that D has no user-definable default constructors weakens that
argument 
 (and is a weakness of the language itself that we'd do good to fix).
Hence our disagreement :-) BTW, over the years I've been evolving towards the notion that exception handling is a mistake.
Jun 09
prev sibling next sibling parent reply Max Samukha <maxsamukha gmail.com> writes:
On Wednesday, 9 June 2021 at 02:37:34 UTC, Walter Bright wrote:
 On 6/6/2021 4:54 AM, Mathias LANG wrote:
 https://github.com/dlang/druntime/pull/3476/files
Frankly, the whole jazz with assert() went way off the rails. Assert should go directly to Jail, it should not pass Go or collect $200. All these layers it goes through is just madness. Allocating via the GC is in assert is just ridiculous, as the program is going to exit shortly anyway. There won't ever be a need to run a collection on it. Just malloc away and fuggeddabootet. These sorts of things are why I added the checkaction switch to bypass it and just do the simple, direct stuff. --- Throwing constructors is one of those things that makes a program very hard to reason about.
Placing (part of) constructor logic into a separate function that may throw, and pretending that a partially constructed state is a valid state - why do you think it is better?
 There's not a single throwing constructor in DMD, and it's 
 going to stay that way :-)
Yeah, because they are not constructors. I bet you either construct lazily when a throwing public method is called, or require an explicit call to a throwing pseudo-constructor.
Jun 08
parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 6/8/2021 11:58 PM, Max Samukha wrote:
 Placing (part of) constructor logic into a separate function that may throw,
and 
 pretending that a partially constructed state is a valid state - why do you 
 think it is better?
As I remarked, it makes the code easier to reason about.
Jun 09
parent Max Samukha <maxsamukha gmail.com> writes:
On Wednesday, 9 June 2021 at 21:40:45 UTC, Walter Bright wrote:

 As I remarked, it makes the code easier to reason about.
Sounds unreasonable to me.
Jun 12
prev sibling next sibling parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 6/8/21 10:37 PM, Walter Bright wrote:
 On 6/6/2021 4:54 AM, Mathias LANG wrote:
  > https://github.com/dlang/druntime/pull/3476/files
 
 Frankly, the whole jazz with assert() went way off the rails. Assert 
 should go directly to Jail, it should not pass Go or collect $200. All 
 these layers it goes through is just madness.
 
 Allocating via the GC is in assert is just ridiculous, as the program is 
 going to exit shortly anyway. There won't ever be a need to run a 
 collection on it. Just malloc away and fuggeddabootet.
Come to think of it, an InvalidMemoryOperationError should use malloc instead of GC, then maybe we can get stack traces? Note that fixing asserts so they don't GC-allocate is just masking what the true problem here is -- any InvalidMemoryOperationError reports zero context for why it was thrown. In *this case* it happened to be an assert inside a destructor. But there are other times where an inadvertent destructor allocation causes an error, and it's impossible to find the spot where it was triggered without using a debugger. -Steve
Jun 09
parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 6/9/21 7:02 AM, Steven Schveighoffer wrote:
 Come to think of it, an InvalidMemoryOperationError should use malloc 
 instead of GC, then maybe we can get stack traces?
I delved into this a bit, trying to see where the GC allocations are happening. There are some functions that are not marked nogc that can be. I got hung up on core.demangle.demangle. It's used here: https://github.com/dlang/druntime/blob/751f5665a31ea0f150d71b22a9852abacc2f61ec/src/core/runtime.d#L830-L865 Ironically, it demangles the string into a buffer, and then truncates that allocated buffer if it was allocated on the heap into the static buffer (and essentially throws away all the work if truncated). We can possibly fix this by telling the demangler to use malloc (maybe in a nogc version), or tell the demangler to truncate for us if it runs out of room in the buffer (preferred). The demangler is quite complex, I tried something cute by making a type that is a fake unlimited size array (but doesn't write to anything past the buffer), but it was more trouble than it's worth. I'm 99% sure we could have a nogc stack trace for InvalidMemoryOperationError if we had a nogc demangler. Everything else seems already nogc, just not fully marked. -Steve
Jun 09
next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 6/9/2021 7:01 AM, Steven Schveighoffer wrote:
 I'm 99% sure we could have a nogc stack trace for InvalidMemoryOperationError
if 
 we had a nogc demangler. Everything else seems already nogc, just not fully
marked.
I don't believe it would be hard to make a nogc demangler, even if it used malloc. Even better, provide a "sink" parameter for the result, and have the demangler defer its final allocation needs to the caller, and free its temporary data itself.
Jun 09
next sibling parent max haughton <maxhaton gmail.com> writes:
On Wednesday, 9 June 2021 at 21:44:20 UTC, Walter Bright wrote:
 On 6/9/2021 7:01 AM, Steven Schveighoffer wrote:
 I'm 99% sure we could have a nogc stack trace for 
 InvalidMemoryOperationError if we had a nogc demangler. 
 Everything else seems already nogc, just not fully marked.
I don't believe it would be hard to make a nogc demangler, even if it used malloc. Even better, provide a "sink" parameter for the result, and have the demangler defer its final allocation needs to the caller, and free its temporary data itself.
The issue with the druntime demangler at the moment seems to be that it basically implements a state machine (pretty normal so far) then uses exceptions to backtrack on itself. The code is not good.
Jun 09
prev sibling parent Mathias LANG <geod24 gmail.com> writes:
On Wednesday, 9 June 2021 at 21:44:20 UTC, Walter Bright wrote:
 On 6/9/2021 7:01 AM, Steven Schveighoffer wrote:
 I'm 99% sure we could have a nogc stack trace for 
 InvalidMemoryOperationError if we had a nogc demangler. 
 Everything else seems already nogc, just not fully marked.
I don't believe it would be hard to make a nogc demangler, even if it used malloc. Even better, provide a "sink" parameter for the result, and have the demangler defer its final allocation needs to the caller, and free its temporary data itself.
Yep, the sink approach seems the most practical one, from my experience. We use it a lot (for `toString`, serialization, hashing...) but it has one major drawback: It is restricted to the attributes that are set by the sink / function. I talked about this when I mentioned [my proposal for ADA at last DConf](https://youtu.be/9lOtOtiwXY4?t=416) and I have a [PR for a DIP I need to finish](https://github.com/dlang/DIPs/pull/198). But for the demangler, we could just manually provide all possible overloads and call it a day. Regarding the bigger point, I've been wanting to have more structured stack traces for a very long time, and that would also enable ` nogc` stack traces. Currently, [object.Throwable.TraceInfo](https://github.com/dlang/druntime/blob/751f5665a31ea0f150d71b22a9852abacc2f61ec/src/ob ect.d#L2257-L2262), the interface exposed by the runtime, is the other blocker, since any change would be a breaking change, its `toString` doesn't take a sink, and its `opApply` provides a formatted string, which forces allocation in `opApply`. Currently, this is worked around by having a limited buffer (https://github.com/dlang/druntime/blob/751f5665a31ea0f150d71b22a9852abacc2f61ec/src/core/internal/backtrac /dwarf.d#L197-L239) but it just means we have a length limit for stack traces. I really hope we can expose [a struct like this](https://github.com/dlang/druntime/blob/751f5665a31ea0f150d71b22a9852abacc2f61ec/src/core/internal/backtra e/dwarf.d#L77-L160) to `TraceInfo` so that users can iterate on the stack trace themselves.
Jun 09
prev sibling parent reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Wednesday, 9 June 2021 at 14:01:39 UTC, Steven Schveighoffer 
wrote:
 On 6/9/21 7:02 AM, Steven Schveighoffer wrote:
 Come to think of it, an InvalidMemoryOperationError should use 
 malloc instead of GC, then maybe we can get stack traces?
I delved into this a bit, trying to see where the GC allocations are happening. There are some functions that are not marked nogc that can be. I got hung up on core.demangle.demangle. It's used here: https://github.com/dlang/druntime/blob/751f5665a31ea0f150d71b22a9852abacc2f61ec/src/core/runtime.d#L830-L865
Would it be possible to just skip demangling in situations where the GC is unavailable? Pasting the output through [ddemangle](https://github.com/dlang/tools/blob/master/ddemangle.d) isn't hard at all.
Jun 09
parent Steven Schveighoffer <schveiguy gmail.com> writes:
On 6/9/21 11:13 PM, Vladimir Panteleev wrote:
 On Wednesday, 9 June 2021 at 14:01:39 UTC, Steven Schveighoffer wrote:
 On 6/9/21 7:02 AM, Steven Schveighoffer wrote:
 Come to think of it, an InvalidMemoryOperationError should use malloc 
 instead of GC, then maybe we can get stack traces?
I delved into this a bit, trying to see where the GC allocations are happening. There are some functions that are not marked nogc that can be. I got hung up on core.demangle.demangle. It's used here: https://github.com/dlang/druntime/blob/751f5665a31ea0f150d71b22a9852abacc2f61ec/src/core runtime.d#L830-L865
Would it be possible to just skip demangling in situations where the GC is unavailable? Pasting the output through [ddemangle](https://github.com/dlang/tools/blob/master/ddemangle.d) isn't hard at all.
Yeah, actually, that's a good idea. It's better than no stack trace, and with GC.inFinalizer, we have the ability to know when we can use demangle or not in the general case. A further update can fix the demangler, and then the printouts just get better. Since this is all part of the traceinfo, just fixing the traceinfo for that exception (which is custom anyway) is possible for now. I'll see if I can make a PR. -Steve
Jun 10
prev sibling next sibling parent reply Timon Gehr <timon.gehr gmx.ch> writes:
On 09.06.21 04:37, Walter Bright wrote:
 
 Allocating via the GC is in assert is just ridiculous, as the program is 
 going to exit shortly anyway. There won't ever be a need to run a 
 collection on it. Just malloc away and fuggeddabootet.
AssertError is still *caught* /by the language/ on in contract inheritance.
Jun 09
next sibling parent Walter Bright <newshound2 digitalmars.com> writes:
On 6/9/2021 10:54 AM, Timon Gehr wrote:
 AssertError is still *caught* /by the language/ on in contract inheritance.
That's a fault in the language design. I do make a mistake now and then :-/ All I can do is grovel and beg forgiveness.
Jun 09
prev sibling next sibling parent reply Kagamin <spam here.lot> writes:
On Wednesday, 9 June 2021 at 17:54:06 UTC, Timon Gehr wrote:
 On 09.06.21 04:37, Walter Bright wrote:
 
 Allocating via the GC is in assert is just ridiculous, as the 
 program is going to exit shortly anyway. There won't ever be a 
 need to run a collection on it. Just malloc away and 
 fuggeddabootet.
AssertError is still *caught* /by the language/ on in contract inheritance.
I'd say, lower assert failures in contracts to a different function, say, _d_contractFailed(), which can do a different thing, say, throw ContractError, and child contract can catch only this specific ContractError instead of any Error.
Jun 10
next sibling parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 6/10/21 8:07 AM, Kagamin wrote:
 On Wednesday, 9 June 2021 at 17:54:06 UTC, Timon Gehr wrote:
 On 09.06.21 04:37, Walter Bright wrote:
 Allocating via the GC is in assert is just ridiculous, as the program 
 is going to exit shortly anyway. There won't ever be a need to run a 
 collection on it. Just malloc away and fuggeddabootet.
AssertError is still *caught* /by the language/ on in contract inheritance.
I'd say, lower assert failures in contracts to a different function, say, _d_contractFailed(), which can do a different thing, say, throw ContractError, and child contract can catch only this specific ContractError instead of any Error.
What about asserts in functions called by the contract? I guess technically they are not part of the contract, just poor coding, but people may rely on those asserts in some cases. -Steve
Jun 10
parent reply FeepingCreature <feepingcreature gmail.com> writes:
On Thursday, 10 June 2021 at 12:59:59 UTC, Steven Schveighoffer 
wrote:
 On 6/10/21 8:07 AM, Kagamin wrote:
 On Wednesday, 9 June 2021 at 17:54:06 UTC, Timon Gehr wrote:
 On 09.06.21 04:37, Walter Bright wrote:
 Allocating via the GC is in assert is just ridiculous, as 
 the program is going to exit shortly anyway. There won't 
 ever be a need to run a collection on it. Just malloc away 
 and fuggeddabootet.
AssertError is still *caught* /by the language/ on in contract inheritance.
I'd say, lower assert failures in contracts to a different function, say, _d_contractFailed(), which can do a different thing, say, throw ContractError, and child contract can catch only this specific ContractError instead of any Error.
What about asserts in functions called by the contract? I guess technically they are not part of the contract, just poor coding, but people may rely on those asserts in some cases. -Steve
All the more reason to throw `ContractError` instead of `AssertError`. Asserts in an unrelated function should not be caught as part of in-condition processing.
Jun 10
parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 6/10/21 9:09 AM, FeepingCreature wrote:
 On Thursday, 10 June 2021 at 12:59:59 UTC, Steven Schveighoffer wrote:
 On 6/10/21 8:07 AM, Kagamin wrote:
 On Wednesday, 9 June 2021 at 17:54:06 UTC, Timon Gehr wrote:
 On 09.06.21 04:37, Walter Bright wrote:
 Allocating via the GC is in assert is just ridiculous, as the 
 program is going to exit shortly anyway. There won't ever be a need 
 to run a collection on it. Just malloc away and fuggeddabootet.
AssertError is still *caught* /by the language/ on in contract inheritance.
I'd say, lower assert failures in contracts to a different function, say, _d_contractFailed(), which can do a different thing, say, throw ContractError, and child contract can catch only this specific ContractError instead of any Error.
What about asserts in functions called by the contract? I guess technically they are not part of the contract, just poor coding, but people may rely on those asserts in some cases.
All the more reason to throw `ContractError` instead of `AssertError`. Asserts in an unrelated function should not be caught as part of in-condition processing.
What about asserts in a related function? The foundation of programming with functions is that you can abstract common implementation into another function. I could easily see someone making a helper function for lots of similar contracts. It's an easy fix, just use to-be-implemented "contract_assert" which throws the right error, but the change would break code that exists. A deprecation might be painful (probably only triggered at runtime, so you might not catch all your cases). -Steve
Jun 10
parent reply Kagamin <spam here.lot> writes:
On Thursday, 10 June 2021 at 14:07:04 UTC, Steven Schveighoffer 
wrote:
 What about asserts in a related function? The foundation of 
 programming with functions is that you can abstract common 
 implementation into another function.

 I could easily see someone making a helper function for lots of 
 similar contracts.
Maybe move all that logic into the precondition of that function? void businessFunction(a,b,c) in { checkContract(a,b,c); } {...} void checkContract(a,b,c) in { lots of asserts } { empty }
Jun 10
parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 6/10/21 1:09 PM, Kagamin wrote:
 On Thursday, 10 June 2021 at 14:07:04 UTC, Steven Schveighoffer wrote:
 What about asserts in a related function? The foundation of 
 programming with functions is that you can abstract common 
 implementation into another function.

 I could easily see someone making a helper function for lots of 
 similar contracts.
Maybe move all that logic into the precondition of that function? void businessFunction(a,b,c) in { checkContract(a,b,c); } {...} void checkContract(a,b,c) in { lots of asserts } { empty }
You're not wrong, but it doesn't fix existing code which might not have done it that way. -Steve
Jun 10
next sibling parent reply Kagamin <spam here.lot> writes:
On Thursday, 10 June 2021 at 17:53:17 UTC, Steven Schveighoffer 
wrote:
 You're not wrong, but it doesn't fix existing code which might 
 not have done it that way.
Now that I think about it, array bounds error is an array contract error, which will make most errors contract errors, with little left for other errors.
Jun 10
parent sighoya <sighoya gmail.com> writes:
On Thursday, 10 June 2021 at 19:20:23 UTC, Kagamin wrote:

 Now that I think about it, array bounds error is an array 
 contract error, which will make most errors contract errors, 
 with little left for other errors.
Contracts are not a mandatory part of the semantic, they can be wiped out leading to inconsistencies when executing code. Therefore, we still need the other errors. Anyway, why should an OutOfBounds error not be a subtype of an AssertionError?
Jun 10
prev sibling parent Ola Fosheim =?UTF-8?B?R3LDuHN0YWQ=?= <ola.fosheim.grostad gmail.com> writes:
On Thursday, 10 June 2021 at 17:53:17 UTC, Steven Schveighoffer 
wrote:
 You're not wrong, but it doesn't fix existing code which might 
 not have done it that way.
The whole idea of executing imperative code in contracts is pretty daft to begin with, makes it much harder for the optimizer to get rid of it. Ideally a contract should just be a simple boolean expression that the calling context would get rid of by proof/optimization. How many people use contracts as implemented?
Jun 10
prev sibling parent Tim <tim.dlang t-online.de> writes:
On Thursday, 10 June 2021 at 12:07:34 UTC, Kagamin wrote:
 I'd say, lower assert failures in contracts to a different 
 function, say, _d_contractFailed(), which can do a different 
 thing, say, throw ContractError, and child contract can catch 
 only this specific ContractError instead of any Error.
This could also enable a different use case: Testing functions with random inputs, but ignoring failures for invalid inputs. void randomTest(alias F)() { while(true) { Parameters!F args; generateRandomArgs!(args); try { F(args); } catch(ContractError) { // Ignore } catch(Exception) { // Ignore } } } This could find bugs, where an assert or bounds check fails in the function.
Jun 10
prev sibling parent reply FeepingCreature <feepingcreature gmail.com> writes:
On Wednesday, 9 June 2021 at 17:54:06 UTC, Timon Gehr wrote:
 On 09.06.21 04:37, Walter Bright wrote:
 
 Allocating via the GC is in assert is just ridiculous, as the 
 program is going to exit shortly anyway. There won't ever be a 
 need to run a collection on it. Just malloc away and 
 fuggeddabootet.
AssertError is still *caught* /by the language/ on in contract inheritance.
Note that -preview=inclusiveincontracts changes the semantics so that AssertError is, while still caught, never *swallowed* but at most converted into another Error. In other words, if the parent in-condition throws, the child in-condition will always throw as well. For discussion of this feature, see https://github.com/dlang/dmd/pull/11465 . I'm sorry I haven't had time to push on this recently.
Jun 10
parent FeepingCreature <feepingcreature gmail.com> writes:
On Thursday, 10 June 2021 at 12:49:19 UTC, FeepingCreature wrote:
 Note that -preview=inclusiveincontracts changes the semantics 
 so that AssertError is, while still caught, never *swallowed* 
 but at most converted into another Error. In other words, if 
 the parent in-condition throws, the child in-condition will 
 always throw as well.

 For discussion of this feature, see 
 https://github.com/dlang/dmd/pull/11465 .
Sorry, let me correct that and explain a bit more. The semantics change of "inclusive in-contracts" is that the in-contract of the child has to include the in-contract of the parent in the course of expanding it. In other words, you can't override `foo(int i) in (i == 2)` with `foo(int i) in (i == 3)`, but only with `foo(int i) in (i == 2 || i == 3)`. This is validated by first checking the child contract, then if it fails, recursively checking that the parent also failed. In other words, the child's error is caught, but the handler then either throws the parent's error or a `LogicError`. So with this change, `assert` can violate ` nogc` as it wants, because the program cannot get out of throwing an `Error` one way or another.
Jun 10
prev sibling parent reply Mathias LANG <geod24 gmail.com> writes:
On Wednesday, 9 June 2021 at 02:37:34 UTC, Walter Bright wrote:
 On 6/6/2021 4:54 AM, Mathias LANG wrote:
 https://github.com/dlang/druntime/pull/3476/files
Frankly, the whole jazz with assert() went way off the rails. Assert should go directly to Jail, it should not pass Go or collect $200. All these layers it goes through is just madness. Allocating via the GC is in assert is just ridiculous, as the program is going to exit shortly anyway. There won't ever be a need to run a collection on it. Just malloc away and fuggeddabootet. These sorts of things are why I added the checkaction switch to bypass it and just do the simple, direct stuff.
Actually we use `-checkaction=context` by default in our unittest build and it allocates even more :) But yeah, we should *not* consider `assert` as recoverable. Which means we should not: 1) Catch them in contracts; 2) [Have unittests for assertion failures](https://github.com/dlang/phobos/blob/815a718c81f15e3155f95260b299ce1b7887664d/std/typecons.d#L3141-L3153) Regarding point 2, we even have [special code](https://github.com/bosagora/agora/blob/fc210b69a6d8e9777935e6e368f318434e3df466/source/agora/te t/Base.d#L118-L121) to handle this. Note that our application logs to a buffer in unittest mode, and we dump that buffer on assertion failure, so successful runs are silent and failing runs give you the full context.
 Throwing constructors is one of those things that makes a 
 program very hard to reason about. I've debated with Andrei 
 about requiring that constructors be nothrow. My advice is make 
 them nothrow, i.e. design them so they cannot fail. There's not 
 a single throwing constructor in DMD, and it's going to stay 
 that way :-)
Last time I checked, there's *nothing* that was throwing in DMD?
Jun 09
parent max haughton <maxhaton gmail.com> writes:
On Thursday, 10 June 2021 at 00:39:44 UTC, Mathias LANG wrote:
 On Wednesday, 9 June 2021 at 02:37:34 UTC, Walter Bright wrote:
 [...]
Actually we use `-checkaction=context` by default in our unittest build and it allocates even more :) [...]
There aren't that many constructors in general, lots of static methods / construct after variable declaration-ing.
Jun 09
prev sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 6/6/21 7:54 AM, Mathias LANG wrote:
 Over the past few months, more than half of the critical bug we've 
 encountered which were not due to our own code are related to 
 destructors. Specifically, destructors being invoked by the GC.
Code in class destructors needs special typechecking and be restricted. IIRC, among other things it wasn't supposed to assume references to other class objects are valid. This has been discussed several times but never implemented.
Jun 08
parent Walter Bright <newshound2 digitalmars.com> writes:
On 6/8/2021 9:50 PM, Andrei Alexandrescu wrote:
 Code in class destructors needs special typechecking and be restricted. IIRC, 
 among other things it wasn't supposed to assume references to other class 
 objects are valid. This has been discussed several times but never implemented.
Probably because nobody ever filed a bugzilla entry for it?
Jun 09