Skip to content

Comments

Make the allocators' unittests and allocate, owns and resolveInternal…#5330

Closed
edi33416 wants to merge 3 commits intodlang:masterfrom
edi33416:allocators_safe_allocation
Closed

Make the allocators' unittests and allocate, owns and resolveInternal…#5330
edi33416 wants to merge 3 commits intodlang:masterfrom
edi33416:allocators_safe_allocation

Conversation

@edi33416
Copy link
Contributor

…Pointer methods safe

Allocating memory should be a safe operation and so should functions like owns and resolveInternalPointer.

{

size_t goodAllocSize(size_t s)
@safe size_t goodAllocSize(size_t s)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no guarantee that parent.goodAllocSize is @safe. Best leave this to be inferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imho I think that this function should be safe and thus I thought we should enforce it upon the parent.


static if (hasMember!(Allocator, "owns"))
Ternary owns(void[] b)
@safe Ternary owns(void[] b)
Copy link
Contributor

Choose a reason for hiding this comment

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

same problem here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same for this

@JackStouffer
Copy link
Contributor

Under no circumstances should @trusted be applied to an entire function.

@aG0aep6G
Copy link
Contributor

I've protested the change to ubyte[] with code that looks innocuous but actually causes memory corruption (see here). You're now making that code @safe.

import core.memory: GC;
import std.stdio: writeln;
import std.experimental.allocator: make;
import std.experimental.allocator.building_blocks: NullAllocator, Region;

struct S { int* p; }

void main() @safe /* !! */
{
    auto buf = new ubyte[](S.sizeof);
    auto r = Region!(NullAllocator, 1)(buf);
    S* s = r.make!S(new int);
    *(*s).p = 42;
    writeln(*(*s).p); /* prints "42" as expected */
    
    version (none) GC.collect(); /* not @safe */
    else new int[](1_000); /* trigger a GC collection in an @safe manner */
    
    *(new int) = 13;
    writeln(*(*s).p); /* prints "13" - whoopsie */
}

This is exactly what must not happen in @safe code.

@andralex
Copy link
Member

OK @edi33416 and I just talked about this and we'll go with the following idea - define this struct:

struct OpaquePointer
{
    private void* pointer;
    @disable this(void*);
}

Now, new OpaquePointer[100] will mark the memory for scanning by the GC.

Allocators that manage a raw buffer of memory will accept OpaquePointer[] safely. The responsbility of allocating the buffer properly falls on the caller.

@edi33416 you may want to shorten this PR to only include the other, simpler primitives.

@andralex
Copy link
Member

this is being sold as parts :)

@andralex andralex closed this Oct 24, 2017
@edi33416 edi33416 mentioned this pull request Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants