Skip to content

Make allocate @safe #5879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
edi33416 wants to merge 1 commit into from
Closed

Make allocate @safe #5879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 24 additions & 19 deletions std/experimental/allocator/building_blocks/affix_allocator.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,17 +124,19 @@ struct AffixAllocator(Allocator, Prefix, Suffix = void)
if (!bytes) return null;
auto result = parent.allocate(actualAllocationSize(bytes));
if (result is null) return null;
static if (stateSize!Prefix)
{
assert(result.ptr.alignedAt(Prefix.alignof));
emplace!Prefix(cast(Prefix*) result.ptr);
}
static if (stateSize!Suffix)
{
auto suffixP = result.ptr + result.length - Suffix.sizeof;
assert(suffixP.alignedAt(Suffix.alignof));
emplace!Suffix(cast(Suffix*)(suffixP));
}
() @trusted {
static if (stateSize!Prefix)
{
assert(result.ptr.alignedAt(Prefix.alignof));
emplace!Prefix(cast(Prefix*) result.ptr);
Copy link
Contributor

@aG0aep6G aG0aep6G Nov 21, 2017

Choose a reason for hiding this comment

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

This assumes that it's safe to write over result. As far as I see, you can't assume that. Every array type implicitly converts to void[], so it's possible that parent.allocate returns memory that's already used elsewhere, differently typed.

Same with Suffix below.

Copy link
Contributor Author

@edi33416 edi33416 Nov 21, 2017

Choose a reason for hiding this comment

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

Isn't it safe to assume that parent.allocate will return fresh memory? Imho, allocate should always return fresh memory.

Copy link
Contributor

@aG0aep6G aG0aep6G Nov 22, 2017

Choose a reason for hiding this comment

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

Isn't it safe to assume that parent.allocate will return fresh memory?

No. parent is provided by the user. You can't assume anything about it beyond what is guaranteed/required by the language.

It might be possible to check if the return value is unique. Then you can rely on that. But you have to check.

Copy link
Member

@andralex andralex Nov 30, 2017

Choose a reason for hiding this comment

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

This is problematic. We may need to replace the result of allocate with ubyte[] everywhere, or give up on making allocate safe for this allocator.

}
static if (stateSize!Suffix)
{
auto suffixP = result.ptr + result.length - Suffix.sizeof;
assert(suffixP.alignedAt(Suffix.alignof));
emplace!Suffix(cast(Suffix*)(suffixP));
}
}();
return result[stateSize!Prefix .. stateSize!Prefix + bytes];
}

Expand Down Expand Up @@ -411,18 +413,20 @@ struct AffixAllocator(Allocator, Prefix, Suffix = void)
auto a = AffixAllocator!(BitmappedBlock!128, ulong, ulong)
(BitmappedBlock!128(new ubyte[128 * 4096]));
assert((() pure nothrow @safe @nogc => a.empty)() == Ternary.yes);
auto b = a.allocate(42);
auto b = (() pure nothrow @safe @nogc => a.allocate(42))();
assert(b.length == 42);
assert((() pure nothrow @safe @nogc => a.empty)() == Ternary.no);
}

@system unittest
nothrow @safe @nogc unittest
{
import std.experimental.allocator.mallocator : Mallocator;
alias A = AffixAllocator!(Mallocator, size_t);
auto b = A.instance.allocate(10);
A.instance.prefix(b) = 10;
assert(A.instance.prefix(b) == 10);
() @trusted {
A.instance.prefix(b) = 10;
assert(A.instance.prefix(b) == 10);
}();

import std.experimental.allocator.building_blocks.null_allocator
: NullAllocator;
Expand Down Expand Up @@ -454,18 +458,19 @@ struct AffixAllocator(Allocator, Prefix, Suffix = void)
assert(p.ptr is d.ptr && p.length >= d.length);
}

@system unittest
//@system unittest
nothrow @safe unittest
{
import std.experimental.allocator.gc_allocator;
alias a = AffixAllocator!(GCAllocator, uint).instance;

// Check that goodAllocSize inherits from parent, i.e. GCAllocator
assert(__traits(compiles, (() nothrow @safe @nogc => a.goodAllocSize(1))()));
assert(__traits(compiles, (() @nogc => a.goodAllocSize(1))()));
Copy link
Contributor

@wilzbach wilzbach Nov 21, 2017

Choose a reason for hiding this comment

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

This is an anti-pattern. Get rid of the __traits(compiles here.

Copy link
Member

@n8sh n8sh Dec 8, 2017

Choose a reason for hiding this comment

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

@wilzbach What is the preferred alternative to __traits(compiles, ...)?

Copy link
Contributor

@wilzbach wilzbach Dec 8, 2017

Choose a reason for hiding this comment

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

a.goodAllocSize(1) and if don't want to call it, simply put it in a function or lambda which isn't called. It looks a lot nicer to read and has the same effect and for the main reason you see the error messages e.g. if you decide to refactor your codebase.

FWIW it should have been static assert too.


// Ensure deallocate inherits from parent
auto b = a.allocate(42);
assert(b.length == 42);
() nothrow @nogc { a.deallocate(b); }();
() @trusted @nogc { a.deallocate(b); }();
Copy link
Contributor

@wilzbach wilzbach Nov 21, 2017

Choose a reason for hiding this comment

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

Where did the nothrow go? And better wrap the specific part in an @safe lambda than spreading @trusted

Copy link
Contributor Author

@edi33416 edi33416 Dec 1, 2017

Choose a reason for hiding this comment

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

Where did the nothrow go?

I changed the unittest to nothrow @safe

And better wrap the specific part in an @safe lambda than spreading @trusted

What do you mean?

}

// Test that deallocateAll infers from parent
Expand All @@ -474,7 +479,7 @@ struct AffixAllocator(Allocator, Prefix, Suffix = void)
import std.experimental.allocator.building_blocks.region : Region;

auto a = AffixAllocator!(Region!(), uint)(Region!()(new ubyte[1024 * 64]));
auto b = a.allocate(42);
auto b = (() pure nothrow @safe @nogc => a.allocate(42))();
assert(b.length == 42);
assert((() nothrow @nogc => a.deallocateAll())());
}
126 changes: 83 additions & 43 deletions std/experimental/allocator/building_blocks/allocator_list.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ struct AllocatorList(Factory, BookkeepingAllocator = GCAllocator)
@disable this(this);

// Is this node unused?
void setUnused() { next = &this; }
bool unused() const { return next is &this; }
void setUnused() @trusted { next = &this; }
bool unused() @safe const { return next is &this; }

// Just forward everything to the allocator
alias a this;
Expand Down Expand Up @@ -196,9 +196,9 @@ struct AllocatorList(Factory, BookkeepingAllocator = GCAllocator)

private void moveAllocators(void[] newPlace)
{
assert(newPlace.ptr.alignedAt(Node.alignof));
assert((() @trusted => newPlace.ptr.alignedAt(Node.alignof))());
assert(newPlace.length % Node.sizeof == 0);
auto newAllocators = cast(Node[]) newPlace;
auto newAllocators = (() @trusted => cast(Node[]) newPlace)();
assert(allocators.length <= newAllocators.length);

// Move allocators
Expand All @@ -210,11 +210,11 @@ struct AllocatorList(Factory, BookkeepingAllocator = GCAllocator)
continue;
}
import core.stdc.string : memcpy;
memcpy(&newAllocators[i].a, &e.a, e.a.sizeof);
() @trusted { memcpy(&newAllocators[i].a, &e.a, e.a.sizeof); }();
if (e.next)
{
newAllocators[i].next = newAllocators.ptr
+ (e.next - allocators.ptr);
newAllocators[i].next = (() @trusted => newAllocators.ptr
+ (e.next - allocators.ptr))();
}
else
{
Expand All @@ -230,19 +230,19 @@ struct AllocatorList(Factory, BookkeepingAllocator = GCAllocator)
auto toFree = allocators;

// Change state
root = newAllocators.ptr + (root - allocators.ptr);
root = (() @trusted => newAllocators.ptr + (root - allocators.ptr))();
allocators = newAllocators;

// Free the olden buffer
static if (ouroboros)
{
static if (hasMember!(Allocator, "deallocate")
&& hasMember!(Allocator, "owns"))
deallocate(toFree);
() @trusted { deallocate(toFree); }();
}
else
{
bkalloc.deallocate(toFree);
() @trusted { bkalloc.deallocate(toFree); }();
}
}

Expand All @@ -253,7 +253,8 @@ struct AllocatorList(Factory, BookkeepingAllocator = GCAllocator)
static if (hasMember!(Allocator, "expand")
&& hasMember!(Allocator, "owns"))
{
immutable bool expanded = t && this.expand(t, Node.sizeof);
// TODO: remove trusted once expand is safe
immutable bool expanded = (() @trusted => t && this.expand(t, Node.sizeof))();
}
else
{
Expand All @@ -263,26 +264,63 @@ struct AllocatorList(Factory, BookkeepingAllocator = GCAllocator)
{
import std.c.string : memcpy;
assert(t.length % Node.sizeof == 0);
assert(t.ptr.alignedAt(Node.alignof));
allocators = cast(Node[]) t;
assert((&t[0]).alignedAt(Node.alignof));
allocators = (() @trusted => cast(Node[]) t)();
allocators[$ - 1].setUnused;
auto newAlloc = SAllocator(make(atLeastBytes));
memcpy(&allocators[$ - 1].a, &newAlloc, newAlloc.sizeof);
emplace(&newAlloc);
() @trusted {
// StatsCollector.~this isn't safe
auto newAlloc = SAllocator(make(atLeastBytes));
memcpy(&allocators[$ - 1].a, &newAlloc, newAlloc.sizeof);
emplace(&newAlloc);
}();
}
else
{
immutable toAlloc = (allocators.length + 1) * Node.sizeof
+ atLeastBytes + 128;
auto newAlloc = SAllocator(make(toAlloc));
auto newPlace = newAlloc.allocate(
(allocators.length + 1) * Node.sizeof);
if (!newPlace) return null;
moveAllocators(newPlace);
import core.stdc.string : memcpy;
memcpy(&allocators[$ - 1].a, &newAlloc, newAlloc.sizeof);
emplace(&newAlloc);
assert(allocators[$ - 1].owns(allocators) == Ternary.yes);

// TODO: Please double check below
// Because StatsCollector.~this isn't safe, we need to infer the
// safety of allocate
static if (
is(typeof(
() {
// This mustn't compile if allocate is @system
auto newAlloc = SAllocator(make(toAlloc));
() @safe {
// We need this inner lambda because SAllocator.~this
// is @system
auto newPlace = newAlloc.allocate(
(allocators.length + 1) * Node.sizeof);
}();
}()
))
)
{
enum trusted_ = "@trusted";
}
else
{
enum trusted_ = "";
}

bool failedNewAlloc = false;
mixin(q{() } ~ trusted_ ~ q{ {
Copy link
Contributor

@wilzbach wilzbach Dec 1, 2017

Choose a reason for hiding this comment

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

You can do this without a mixin, by simply doing sth. like

static if (<checkWhetherItCanBeTrusted>) 
   () @trusted { callFun() }();
else
   callFun();

auto newAlloc = SAllocator(make(toAlloc));
auto newPlace = newAlloc.allocate(
(allocators.length + 1) * Node.sizeof);
if (!newPlace)
{
failedNewAlloc = true;
return;
}
moveAllocators(newPlace);
import core.stdc.string : memcpy;
memcpy(&allocators[$ - 1].a, &newAlloc, newAlloc.sizeof);
emplace(&newAlloc);
assert(allocators[$ - 1].owns(allocators) == Ternary.yes);
}(); });
if (failedNewAlloc) return null;
}
// Insert as new root
if (root != &allocators[$ - 1])
Expand All @@ -304,29 +342,31 @@ struct AllocatorList(Factory, BookkeepingAllocator = GCAllocator)
{
void[] t = allocators;
static if (hasMember!(BookkeepingAllocator, "expand"))
immutable bool expanded = bkalloc.expand(t, Node.sizeof);
immutable bool expanded = (() @trusted => bkalloc.expand(t, Node.sizeof))();
else
immutable bool expanded = false;
if (expanded)
{
assert(t.length % Node.sizeof == 0);
assert(t.ptr.alignedAt(Node.alignof));
allocators = cast(Node[]) t;
assert((&t[0]).alignedAt(Node.alignof));
allocators = (() @trusted => cast(Node[]) t)();
allocators[$ - 1].setUnused;
}
else
{
// Could not expand, create a new block
t = bkalloc.allocate((allocators.length + 1) * Node.sizeof);
assert(t.length % Node.sizeof == 0);
if (!t.ptr) return null;
if (!t) return null;
moveAllocators(t);
}
assert(allocators[$ - 1].unused);
auto newAlloc = SAllocator(make(atLeastBytes));
import core.stdc.string : memcpy;
memcpy(&allocators[$ - 1].a, &newAlloc, newAlloc.sizeof);
emplace(&newAlloc);
() @trusted {
auto newAlloc = SAllocator(make(atLeastBytes));
import core.stdc.string : memcpy;
memcpy(&allocators[$ - 1].a, &newAlloc, newAlloc.sizeof);
emplace(&newAlloc);
}();
// Creation succeeded, insert as root
if (allocators.length == 1)
allocators[$ - 1].next = null;
Expand Down Expand Up @@ -584,9 +624,9 @@ version(Posix) @system unittest
import std.experimental.allocator.building_blocks.region : Region;
AllocatorList!((n) => Region!GCAllocator(new ubyte[max(n, 1024 * 4096)]),
NullAllocator) a;
const b1 = a.allocate(1024 * 8192);
const b1 = (() @safe => a.allocate(1024 * 8192))();
assert(b1 !is null); // still works due to overdimensioning
const b2 = a.allocate(1024 * 10);
const b2 = (() @safe => a.allocate(1024 * 10))();
assert(b2.length == 1024 * 10);
a.deallocateAll();
}
Expand All @@ -597,9 +637,9 @@ version(Posix) @system unittest
import std.algorithm.comparison : max;
import std.experimental.allocator.building_blocks.region : Region;
AllocatorList!((n) => Region!()(new ubyte[max(n, 1024 * 4096)])) a;
auto b1 = a.allocate(1024 * 8192);
auto b1 = (() @safe => a.allocate(1024 * 8192))();
assert(b1 !is null); // still works due to overdimensioning
b1 = a.allocate(1024 * 10);
b1 = (() @safe => a.allocate(1024 * 10))();
assert(b1.length == 1024 * 10);
a.deallocateAll();
}
Expand All @@ -611,9 +651,9 @@ version(Posix) @system unittest
import std.experimental.allocator.mallocator : Mallocator;
import std.typecons : Ternary;
AllocatorList!((n) => Region!()(new ubyte[max(n, 1024 * 4096)]), Mallocator) a;
auto b1 = a.allocate(1024 * 8192);
auto b1 = (() @safe => a.allocate(1024 * 8192))();
assert(b1 !is null);
b1 = a.allocate(1024 * 10);
b1 = (() @safe => a.allocate(1024 * 10))();
assert(b1.length == 1024 * 10);
a.allocate(1024 * 4095);
assert((() pure nothrow @safe @nogc => a.empty)() == Ternary.no);
Expand All @@ -627,18 +667,18 @@ version(Posix) @system unittest
import std.experimental.allocator.building_blocks.region : Region;
enum bs = GCAllocator.alignment;
AllocatorList!((n) => Region!GCAllocator(256 * bs)) a;
auto b1 = a.allocate(192 * bs);
auto b1 = (() @safe => a.allocate(192 * bs))();
assert(b1.length == 192 * bs);
assert(a.allocators.length == 1);
auto b2 = a.allocate(64 * bs);
auto b2 = (() @safe => a.allocate(64 * bs))();
assert(b2.length == 64 * bs);
assert(a.allocators.length == 1);
auto b3 = a.allocate(192 * bs);
auto b3 = (() @safe => a.allocate(192 * bs))();
assert(b3.length == 192 * bs);
assert(a.allocators.length == 2);
// Ensure deallocate inherits from parent allocators
() nothrow @nogc { a.deallocate(b1); }();
b1 = a.allocate(64 * bs);
b1 = (() @safe => a.allocate(64 * bs))();
assert(b1.length == 64 * bs);
assert(a.allocators.length == 2);
a.deallocateAll();
Expand Down
Loading