Make AssertError a singleton by andralex · Pull Request #1710 · dlang/druntime

andralex · 2016-12-12T15:46:31Z

This makes AssertError a singleton, which allows people to use assert in higher-level applications without needing to link in the garbage collector.

This could break code that calls onAssertError repeatedly and assumes it returns separate objects. That's quite an unusual way to go about things, and it seems we need not guarantee it.

If this approach works we can (a) publish singleton so as to make it available to clients, and (b) convert RangeError to a singleton as well so we can throw range errors without worrying about memory allocations.

mathias-lang-sociomantic · 2016-12-12T15:49:20Z

src/core/exception.d

-    if( _assertHandler is null )
-        throw new AssertError( file, line );
-    _assertHandler( file, line, null);
+    onAssertErrorMsg(file, line, null);


That should really be done on the compiler level one day

@mathias-lang-sociomantic yes, and it will

@WalterBright : Having two function seems unnecessary, as the case of assert(cond) could just be treated as assert(cond, null). That's one less function between DMD and druntime, so less complication.
Also it's a common source of frustration that when debugging we need to break both on _d_assert and _d_assert_msg.

yah, that's what was happening anyway (with duplicated code)

mathias-lang-sociomantic · 2016-12-12T15:50:03Z

src/core/exception.d

 // Overridable Callbacks
 ///////////////////////////////////////////////////////////////////////////////

+private C singleton(C)() if (is(C == class) && is(typeof(new C)))


Documentation and unittests please

unittests are there... documentation not needed for private functions :)

andralex · 2016-12-12T16:03:34Z

Added support for destruction via atexit.

andralex · 2016-12-12T16:03:45Z

,,, @mathias-lang-sociomantic and dox :)

mathias-lang-sociomantic · 2016-12-12T16:12:20Z

src/core/exception.d

+        assert(typeid(C).initializer.length == b.length * size_t.sizeof);
+        import core.stdc.string;
+        memcpy(b.ptr, typeid(C).init.ptr, b.length);
+        result.__ctor;


Doesn't that mean that the ctor can be @system and do bad stuff ? ™️
Ditto for the dtor.

you're right brb

tsbockman · 2016-12-12T19:16:54Z

This makes AssertError a singleton

When I read that, I immediately wondered about the potential race condition if multiple threads were to assert simultaneously. Then I saw:

repeated calls to singleton!C within a given thread return the same instance

Is it correct to refer to an object as a "singleton" when there's actually one per thread? It's very confusing, etymologically speaking.

andralex · 2016-12-12T20:00:50Z

@tsbockman good point thx, changed

WalterBright · 2016-12-12T21:46:45Z

src/core/exception.d


+/*
+Returns a perThreadInstance object of class type `C`, i.e. repeated calls to
+`perThreadInstance!C` within a given thread return the same instance. For that reason,


This is not the standard Ddoc function documentation style.

At this point the function is private so I just added a casual comment.

added nicer dox

mathias-lang-sociomantic · 2016-12-12T23:35:56Z

src/core/exception.d

+        result.__ctor;
+        import core.internal.traits;
+        static if (hasElaborateDestructor!C)
+        {


According to a quick test:

import std.traits, core.exception; static assert(hasElaborateDestructor!AssertError); // Error: static assert (hasElaborateDestructor!(AssertError)) is false

So it would be nice to have this line covered. Otherwise, LGTM.

Thanks. I think we can defer coverage of that to the time we get to use this private function elsewhere.

andralex · 2016-12-12T23:49:54Z

@mathias-lang-sociomantic added coverage

dushibaiyu · 2016-12-13T03:21:29Z

I think other Error should be singleton too.

PetarKirov · 2016-12-12T19:44:06Z

src/core/exception.d

+        result.__ctor;
+        import core.internal.traits;
+        static if (hasElaborateDestructor!C)
+        {


hasElaborateDestructor!C == false if C is not a struct. That's why I proposed dlang/phobos#4119.

PetarKirov · 2016-12-12T19:46:43Z

src/core/exception.d

+    static bool initialized = false;
+    auto result = () @trusted { return cast(C) b.ptr; }();
+    if (!initialized)
+    {


Why not do an early exit:

if (initialized) return result;

Is it just because you don't want to type return result; twice?

I'm glad you're giving the same advice I often do :). I'd say when the if is short things are fine either way.

PetarKirov · 2016-12-13T07:34:11Z

src/core/exception.d

+Throws: If the default constructor of `C` throws.
+*/
+private C perThreadInstance(C)() if (is(C == class) && is(typeof(new C)))
+{


There already is a staticError which covers pretty much the same use case. Perhaps you can reuse it / replace it / unify the implementations?

PetarKirov · 2016-12-13T07:43:12Z

src/core/exception.d

+    static size_t[1 + (__traits(classInstanceSize, C) - 1) / size_t.sizeof] b;
+    static bool initialized = false;
+    auto result = () @trusted { return cast(C) b.ptr; }();
+    if (!initialized)


Since we're druntime, I think it's worth going the extra mile (i.e. do the necessary safe-ty hacks - see line 760) in order to return immutable C or tag the function with pure. @nogc, pure and nothrow are orthogonal in the language and therefore I think it's a good idea to allow @nogc pure functions to throw staticErrors. (To me personally, throw staicError!RangeError sound better, than throw perThreadInstance!RangeError, but naming is not that important at this stage.)

PetarKirov · 2016-12-13T07:47:33Z

src/core/exception.d

+            result.__ctor;
+        import core.internal.traits;
+        static if (hasElaborateDestructor!C)
+        {


hasElaborateDestructor!C always returns false if C is not a struct (even the one in druntime). That's why I proposed dlang/phobos#4119.

Hm... I think just remove this static if. There is always a destructor for a class (i.e. rt_finalize2 will always call destructors, even for Object).

PetarKirov · 2016-12-13T08:14:47Z

src/core/exception.d

+            static void destroy()
+            {
+                (cast(C) b.ptr).__dtor;
+            }


If you don't want to leverage object.destroy, you should at least use rt_finalize2, because that's what object.destroy does for classes.
The problem with calling obj.__dtor directly is that:

It does a non-virtual call (see 1 and 2) to C.~this(), without calling any of C's base classes' dtors.

It does not destroy C's __monitor field.

It does not clear the memory, causing the GC to attempt destroy (this time "properly") the object again - see this (reproduces perfectly on newer compilers too).

Agree, this was actually a problem with destroy before we fixed it.

PetarKirov · 2016-12-13T08:47:45Z

src/core/exception.d

+private C perThreadInstance(C)() if (is(C == class) && is(typeof(new C)))
+{
+    static size_t[1 + (__traits(classInstanceSize, C) - 1) / size_t.sizeof] b;
+    static bool initialized = false;


It would have been simpler to do ubyte[__traits(classInstanceSize, C] b. Why do you want to round up to a multiple of size_t

To be sufficiently paranoid, better add a check for the instance alignment not being larger than size_t then (SIMD vectors, …).

PetarKirov · 2016-12-13T08:52:08Z

src/core/exception.d

+        () @trusted
+        {
+            memcpy(b.ptr, typeid(C).initializer.ptr, b.length);
+        }();


In theory the following could be more efficient auto ci = typeid(C); b[0 .. ci.initializer.length] = ci.initializer[] (assuming b is of type ubyte[]), because the compiler knows at compile time typeid(C).initializer.length and can make a better decision about the codegen (e.g. potentially inline the memcpy for smaller types).

this code will be executed once per run really

PetarKirov · 2016-12-13T10:17:57Z

src/core/exception.d

+        }();
+        static if (is(typeof(result.__ctor())))
+            result.__ctor;
+        import core.internal.traits;


You may need to make sure that the BlkAttr for this piece of memory has the BlkAttr.FINALIZE bit unset, otherwise the GC may attempt to call the dtors again at the end of the program.

Hmmm... this is not GC-allocated memory so it does not have block attributes. No?

@andralex correct.

PetarKirov · 2016-12-28T14:57:11Z

Ping @andralex. I need perThreadInstance to continue my quest on making more of druntime @nogc and nothrow (see #1726 and #1728).

MartinNowak

Please use/adopt staticError, it was build for that purpose.

schveiguy

Looks good. Just need some tweaks to destruction, then it should be good to go.

schveiguy · 2017-05-08T06:45:48Z

src/core/exception.d

+        }();
+        static if (is(typeof(result.__ctor())))
+            result.__ctor;
+        import core.internal.traits;


@andralex correct.

schveiguy · 2017-05-08T06:50:10Z

src/core/exception.d

+            static void destroy()
+            {
+                (cast(C) b.ptr).__dtor;
+            }


Agree, this was actually a problem with destroy before we fixed it.

schveiguy · 2017-05-08T06:53:26Z

src/core/exception.d

+            result.__ctor;
+        import core.internal.traits;
+        static if (hasElaborateDestructor!C)
+        {


Hm... I think just remove this static if. There is always a destructor for a class (i.e. rt_finalize2 will always call destructors, even for Object).

schveiguy · 2017-05-08T06:58:33Z

Note, this will close https://issues.dlang.org/show_bug.cgi?id=4587

schveiguy · 2017-05-08T07:08:18Z

Please use/adopt staticError, it was build for that purpose.

Hm... I think actually staticError may be better off using perThreadInstance. It solves some problems (i.e. the note about chaining), and uses exact memory size vs. guessing for all possible Error types.

@andralex, looking at how staticError works, I think using a void static array may be cleaner than size_t array. two reasons: 1) no math needed for size, so it looks cleaner. 2) precise scanning may care one day.

rainers · 2017-05-08T17:52:33Z

precise scanning may care one day.

It already does, see #1798. For the buffer to still be scanned with proper alignment, it should be an array of void*.

rainers · 2017-05-08T17:57:07Z

I think that there should also be a test showing what happens if an assert triggers during unwinding an AssertError. It doesn't have to support perfect chaining, but it should somehow pass the initial error along.

andralex · 2021-10-31T17:43:55Z

superseded

mathias-lang-sociomantic reviewed Dec 12, 2016

View reviewed changes

Make AssertError a singleton

e144b99

andralex force-pushed the onAssert branch from 5178811 to 9aad0a0 Compare December 12, 2016 16:03

Add dox and call to dtor if defined

b7598eb

mathias-lang-sociomantic reviewed Dec 12, 2016

View reviewed changes

andralex force-pushed the onAssert branch from 9aad0a0 to ec3cad5 Compare December 12, 2016 17:03

Fix use of @trusted

e733c29

andralex force-pushed the onAssert branch from ec3cad5 to e733c29 Compare December 12, 2016 17:05

Change name singleton -> perThreadInstance

ed3fb00

WalterBright reviewed Dec 12, 2016

View reviewed changes

mathias-lang-sociomantic reviewed Dec 12, 2016

View reviewed changes

Add coverage of atexit code

032c00a

Doc

4531dbb

PetarKirov suggested changes Dec 13, 2016

View reviewed changes

PetarKirov reviewed Dec 13, 2016

View reviewed changes

andralex mentioned this pull request Dec 13, 2016

Issue 14294 - partialSort should also accept two ranges dlang/phobos#4947

Merged

Darredevil mentioned this pull request Dec 14, 2016

Fix Issue 4587 - Assert exception should not allocate #1714

Closed

PetarKirov mentioned this pull request Dec 27, 2016

Add attributes to core.thread #1726

Merged

9 tasks

MartinNowak suggested changes Jan 28, 2017

View reviewed changes

PetarKirov mentioned this pull request May 2, 2017

std.experimental.logger: allow precise scanning of DATA/TLS segment dlang/phobos#5357

Merged

schveiguy suggested changes May 8, 2017

View reviewed changes

PetarKirov mentioned this pull request Jun 30, 2017

Annotate with @nogc most of std.datetime.date's API dlang/phobos#5529

Merged

dlang-bot added Needs Rebase needs a `git rebase` performed stalled and removed Needs Rebase needs a `git rebase` performed labels Jan 1, 2018

wilzbach mentioned this pull request Jan 2, 2019

Handle page protection errors as D errors on Linux with --DRT-memoryError=1 #2249

Closed

wilzbach mentioned this pull request Sep 6, 2020

WIP: OS Random number generator interface dlang/phobos#7619

Closed

dlang-bot added Needs Rebase needs a `git rebase` performed Needs Work labels May 27, 2021

andralex closed this Oct 31, 2021

Uh oh!

Comments

Conversation

andralex commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Dec 12, 2016

Uh oh!

andralex commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsbockman commented Dec 12, 2016

Uh oh!

andralex commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Dec 12, 2016

Uh oh!

dushibaiyu commented Dec 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov Dec 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

PetarKirov Dec 13, 2016 •

edited

Loading

schveiguy commented May 8, 2017 •

edited

Loading

rainers commented May 8, 2017 •

edited

Loading