Fix Issue 18848 - std.allocator: Regions are non-copyable, yet are pa…

[CyberShadow]

…ssed around in examples

This doesn't fix that non-copyable regions are still passed around in
examples, so we still rely on NRVO to do its thing and elide the
copy, but at least this will now catch wrong code that mistakenly
copied Regions around.

[dlang-bot]

Thanks for your pull request, @CyberShadow!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18848	normal	std.allocator: Regions are non-copyable, yet are passed around in examples

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#6509"

[CyberShadow]

OK, so, this is bad.

The test failures indicate actual bugs in std.allocator code and unit tests. For example, in the case of FreeList failure, I wrote this program to verify:

import std.experimental.allocator.building_blocks.free_list;
import std.experimental.allocator.common;
import std.stdio;

void logArg(A)(A arg)
{
	static if (is(typeof(arg) : T[], T))
		writef("%s..%s", arg.ptr, arg.ptr + arg.length);
	else
		write(arg);
}

struct LoggingAllocator(ParentAllocator)
{
	static if (stateSize!ParentAllocator)
		ParentAllocator parent;
	else
	{
		alias ParentAllocator.instance parent;
		enum instance = typeof(this).init;
	}

	enum alignment = ParentAllocator.alignment;

	auto opDispatch(string fun, Args...)(auto ref Args args)
	if (is(typeof(mixin(`parent.`~fun~`(args)`))))
	{
		write(fun, "(");
		foreach (i, arg; args)
		{
			if (i) write(", ");
			logArg(arg);
		}
		write(")");
		static if (is(typeof(mixin(`parent.`~fun~`(args)`)) == void))
		{
			mixin(`parent.`~fun~`(args)`);
			writeln;
		}
		else
		{
			auto res = mixin(`parent.`~fun~`(args)`);
			write(" => ");
			logArg(res);
			writeln;
			return res;
		}
	}

	alias parent this;
}

void main()
{
    // Fragment of a FreeList unittest
    import std.experimental.allocator.building_blocks.region : Region;
    import std.experimental.allocator.gc_allocator : GCAllocator;
    import std.typecons : Ternary;
    alias R = Region!(LoggingAllocator!GCAllocator);
    alias A = ContiguousFreeList!(R, 0, 64);
    auto a = A(R(1024 * 4), 1024);

    auto b = a.allocate(100);
    write("Allocated: "); logArg(b); writeln;
}

Currently, the output is:

allocate(4096) => 7F31B70DA000..7F31B70DB000
deallocate(7F31B70DA000..7F31B70DB000) => true
Allocated: 7F31B70DA400..7F31B70DA464
deallocate(7F31B70DA000..7F31B70DB000) => true

Meaning, not only do we "allocate" and return a block inside already-deallocated memory, but there is also a double-free bug (which for some reason GCAllocator ignores).

This indicates that there is a systemic problem in how std.allocator is tested, and there are likely to be more similar bugs lurking around.

I suggest to introduce something like a CheckingAllocator, which sits near the very top of the stack, right below the source allocator (GCAllocator or Mallocator), which checks for such bugs, e.g.:

• That there are no memory leaks (all that was allocated has been deallocated)
• That there are no double-free attempts.

Then, we should make all unit tests use CheckingAllocator.

The failing unit tests themselves should be easy enough to fix, just replace Region!... with Region!...*, and pass a pointer to the Region that'll remain on the stack.

CC @andralex @edi33416

[edi33416]

This is bad. Good catch!

I suggest to introduce something like a CheckingAllocator

We have StatsCollector which we can use; either as it is or to build CheckingAllocator on top of it.

For the current issue, I think that passing a pointer to the Region is a good solution, but the user must be careful not to escape that pointer. Maybe creating an allocatorObject from a Region would be better, because the resulting object will take the ownership of the stack Region allocator; a drawback is that we add a level of indirection.

Regardless of what we decide the approach should be, I think it would be good to disable the postblit for Region and friends and fix the code breakage. Currently this is a sneaky bug.

[edi33416]

CC @jercaianu

[CyberShadow]

We have StatsCollector which we can use; either as it is or to build CheckingAllocator on top of it.

Ah, okay, I guess we can check for double-frees just by making sure we don't deallocate more than we allocated. A double free plus a memory leak shouldn't sum to "all OK", but I guess it doesn't make sense to put the carriage before the horse and overcomplicate things until we run into a bug that requires that level of thoroughness.

For the current issue, I think that passing a pointer to the Region is a good solution, but the user must be careful not to escape that pointer.

Alright, I fixed that instance by making it pass a pointer, and it looks like that was the only instance in the test suite so far, so CI is green.

Maybe creating an allocatorObject from a Region would be better, because the resulting object will take the ownership of the stack Region allocator

Looks like that currently doesn't work, as alignment is aliased to the parent allocator throughout allocator layer types. This creates problems when the parent allocator's alignment is a method and not a constant, in the form of "wrong type of this" compilation errors. I can have a stab at that in another PR.

Regardless of what we decide the approach should be, I think it would be good to disable the postblit for Region and friends and fix the code breakage. Currently this is a sneaky bug.

Agreed, so I think this PR is good for merge.

[CyberShadow]

I can have a stab at that in another PR.

Looks like that wasn't trivial either, started a discussion here:
https://issues.dlang.org/show_bug.cgi?id=18877

[CyberShadow]

Speaking of StatsCollector, we should probably fix this before using it for such purposes:
https://issues.dlang.org/show_bug.cgi?id=18854

[jercaianu]

Thanks for looking into this!
Maybe we should make default practice to wrap all allocators into RCIAllocators to avoid passing by raw pointers.
Any thoughts?

[CyberShadow]

You mean, whenever a pointer would be used, make RCIAllocator the recommended choice? I think that would be fine, though perhaps ideally we'd have better move construction semantics in the language. I think modern C++ doesn't have this problem.

[LightBender]

@thewilsonator this would be a good merge with a resolution.