Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Make AssertError a singleton #1710

Closed
wants to merge 6 commits into from
Closed

Make AssertError a singleton #1710

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e144b99
Make AssertError a singleton
andralex Dec 12, 2016
b7598eb
Add dox and call to dtor if defined
andralex Dec 12, 2016
e733c29
Fix use of @trusted
andralex Dec 12, 2016
ed3fb00
Change name singleton -> perThreadInstance
andralex Dec 12, 2016
032c00a
Add coverage of atexit code
andralex Dec 12, 2016
4531dbb
Doc
andralex Dec 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 74 additions & 6 deletions src/core/exception.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ unittest
*/
class AssertError : Error
{
@safe pure nothrow this()
{
super(null);
}

@safe pure nothrow this( string file, size_t line )
{
this(cast(Throwable)null, file, line);
Expand All @@ -63,8 +68,14 @@ class AssertError : Error
}
}

unittest
@safe unittest
{
{
auto ae = perThreadInstance!AssertError;
assert(typeid(ae) is typeid(AssertError));
assert(ae.file is null);
assert(ae.line == 0);
}
{
auto ae = new AssertError("hello", 42);
assert(ae.file == "hello");
Expand Down Expand Up @@ -416,12 +427,65 @@ deprecated void setAssertHandler( AssertHandler h ) @trusted nothrow @nogc
assertHandler = h;
}

/*
Returns a perThreadInstance object of class type `C`, i.e. repeated calls to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the standard Ddoc function documentation style.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added nicer dox

`perThreadInstance!C` within a given thread return the same instance. For that
reason, no constructor parameters are accepted; `C` must allow default
construction.

Different threads own different instances of `C`.

The destructor of the perThreadInstance object is called during application's
exit.

Returns: A thread-local instance of type `C`.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
static size_t[1 + (__traits(classInstanceSize, C) - 1) / size_t.sizeof] b;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alignment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static bool initialized = false;
auto result = () @trusted { return cast(C) b.ptr; }();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

if (!initialized)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do an early exit:

if (initialized)
    return result;

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
assert(typeid(C).initializer.length == b.length * size_t.sizeof);
import core.stdc.string;
() @trusted
{
memcpy(b.ptr, typeid(C).initializer.ptr, b.length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code will be executed once per run really

}();
static if (is(typeof(result.__ctor())))
result.__ctor;
Copy link
Member

@PetarKirov PetarKirov Dec 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andralex correct.

import core.internal.traits;
static if (hasElaborateDestructor!C)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

{
import core.stdc.stdlib;
static void destroy()
{
(cast(C) b.ptr).__dtor;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
atexit(&destroy);
}
initialized = true;
}
return result;
}

unittest
{
static class C
{
~this() {}
}
// Ensure coverage of the atexit code
assert(perThreadInstance!C !is null);
}

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


/**
* A callback for assert errors in D. The user-supplied assert handler will
* be called if one has been supplied, otherwise an $(LREF AssertError) will be
Expand All @@ -433,9 +497,7 @@ deprecated void setAssertHandler( AssertHandler h ) @trusted nothrow @nogc
*/
extern (C) void onAssertError( string file = __FILE__, size_t line = __LINE__ ) nothrow
{
if( _assertHandler is null )
throw new AssertError( file, line );
_assertHandler( file, line, null);
onAssertErrorMsg(file, line, null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should really be done on the compiler level one day

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mathias-lang-sociomantic yes, and it will

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}


Expand All @@ -452,7 +514,13 @@ extern (C) void onAssertError( string file = __FILE__, size_t line = __LINE__ )
extern (C) void onAssertErrorMsg( string file, size_t line, string msg ) nothrow
{
if( _assertHandler is null )
throw new AssertError( msg, file, line );
{
auto e = perThreadInstance!AssertError;
e.msg = msg;
e.file = file;
e.line = line;
throw e;
}
_assertHandler( file, line, msg );
}

Expand Down