Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive
Merged
Changes from all commits
Commits
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
75 changes: 74 additions & 1 deletion src/object.d
Original file line number Diff line number Diff line change
Expand Up @@ -3514,7 +3514,15 @@ private U[] _dup(T, U)(T[] a) if (!__traits(isPOD, T))
import core.lifetime: copyEmplace;
U[] res = () @trusted {
auto arr = cast(U*) _d_newarrayU(typeid(T[]), a.length);
foreach (i; 0..a.length)
size_t i;
scope (failure)
{
import core.internal.lifetime: emplaceInitializer;
// Initialize all remaining elements to not destruct garbage
foreach (j; i .. a.length)
emplaceInitializer(cast() arr[j]);
Copy link
Contributor

@kinke kinke Jun 11, 2021

Choose a reason for hiding this comment

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

So you're resetting the element whose postblit/cpctor threw - should we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it's necessary because the GC will call the destructor for that element.

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern was that the postblit/cpctor might not clean up itself on an exception and depend on the dtor being run on the partially constructed instance. But that's hopefully handled by -preview=dtorfields, and resetting to T.init correct then. Maybe slightly adjust the comment to make that clear, to prevent someone from thinking it's a one-off bug as I did. ;)

Copy link
Contributor

@John-Colvin John-Colvin Aug 4, 2021

Choose a reason for hiding this comment

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

Isn't the correct thing instead for copyEmplace to reset its destination to T.init on failure, then this loop would be from i + 1? And same for some of the other emplace functions?

Otherwise this fix needs to be done in a bunch of other places because it would mean copyEmplace always needs a try/catch or scope(failure) to use safely if it might throw.

Copy link
Contributor

@kinke kinke Aug 4, 2021

Choose a reason for hiding this comment

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

Yes, that seems to make a lot of sense. After all, regular instances failing their construction aren't destructed, so postblit/cpctor cannot rely on the dtor being run in case they throw. - Maybe just using a try / catch(Exception) to prevent the overhead for nothrow postblits/cpctors/ctors.

Copy link
Contributor

Choose a reason for hiding this comment

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

}
for (; i < a.length; i++)
{
copyEmplace(a.ptr[i], arr[i]);
}
Expand Down Expand Up @@ -3932,6 +3940,71 @@ private void _doPostblit(T)(T[] arr)
auto a = arr.dup; // dup does escape
}

// https://issues.dlang.org/show_bug.cgi?id=21983
// dup/idup destroys partially constructed arrays on failure
@safe unittest
{
static struct SImpl(bool postblit)
{
int num;
long l = 0xDEADBEEF;

static if (postblit)
{
this(this)
{
if (this.num == 3)
throw new Exception("");
}
}
else
{
this(scope ref const SImpl other)
{
if (other.num == 3)
throw new Exception("");

this.num = other.num;
this.l = other.l;
}
}

~this() @trusted
{
if (l != 0xDEADBEEF)
{
import core.stdc.stdio;
printf("Unexpected value: %lld\n", l);
fflush(stdout);
assert(false);
}
}
}

alias Postblit = SImpl!true;
alias Copy = SImpl!false;

static int test(S)()
{
S[4] arr = [ S(1), S(2), S(3), S(4) ];
try
{
arr.dup();
assert(false);
}
catch (Exception)
{
return 1;
}
}

static assert(test!Postblit());
assert(test!Postblit());

static assert(test!Copy());
assert(test!Copy());
}

/**
Destroys the given object and optionally resets to initial state. It's used to
_destroy an object, calling its destructor or finalizer so it no longer
Expand Down