Skip to content

Optimization: Don't generate try-catch for nothrow postblits #8184

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

Merged
RazvanN7 merged 3 commits into from
Apr 28, 2018
Merged

Optimization: Don't generate try-catch for nothrow postblits #8184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
008ce0c
Optimization: Don't generate try-catch for nothrow postblits
JinShil Apr 18, 2018
4427a38
Add field postblit exception safety test
JinShil Apr 18, 2018
8a33fb6
Add test for issue 18493
JinShil Apr 24, 2018
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
136 changes: 85 additions & 51 deletions src/dmd/dsymbolsem.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ private extern (C++) FuncDeclaration buildPostBlit(StructDeclaration sd, Scope*
stc |= sd.postblits[i].storage_class & STC.disable;
}

VarDeclaration[] fieldsToDestroy;
auto postblitCalls = new Statements();
// iterate through all the struct fields that are not disabled
for (size_t i = 0; i < sd.fields.dim && !(stc & STC.disable); i++)
Expand All @@ -116,6 +117,84 @@ private extern (C++) FuncDeclaration buildPostBlit(StructDeclaration sd, Scope*
continue;
assert(!sdv.isUnionDeclaration());

// if this field's postblit is not `nothrow`, add a `scope(failure)`
// block to destroy any prior successfully postblitted fields should
// this field's postblit fail
if (fieldsToDestroy.length > 0 && !(cast(TypeFunction)sdv.postblit.type).isnothrow)
Copy link
Copy Markdown
Contributor Author

@JinShil JinShil Apr 18, 2018

Choose a reason for hiding this comment

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

I think this should actually use the canThrow function, but I can't figure out how to make a proper call to that function. Should I use canThrow here, and if so, how should I construct the call?

Copy link
Copy Markdown
Contributor

@RazvanN7 RazvanN7 Apr 20, 2018

Choose a reason for hiding this comment

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

canThrow is used to check whether an expression can throw, while you have a function which needs to be checked. You could use the constructed call expression to the postblit to see if it's marked as nothrow, but that does essentially the same thing that you are doing here [1], so the current check is fine.

[1]

dmd/src/dmd/canthrow.d

Line 73 in 6371942

if (t.ty == Tfunction && (cast(TypeFunction)t).isnothrow)

{
// create a list of destructors that need to be called
Expression[] dtorCalls;
foreach(sf; fieldsToDestroy)
{
Expression ex;
tv = sf.type.toBasetype();
if (tv.ty == Tstruct)
{
// this.v.__xdtor()

ex = new ThisExp(loc);
ex = new DotVarExp(loc, ex, sf);

// This is a hack so we can call destructors on const/immutable objects.
ex = new AddrExp(loc, ex);
ex = new CastExp(loc, ex, sf.type.mutableOf().pointerTo());
ex = new PtrExp(loc, ex);
if (stc & STC.safe)
stc = (stc & ~STC.safe) | STC.trusted;

auto sfv = (cast(TypeStruct)sf.type.baseElemOf()).sym;

ex = new DotVarExp(loc, ex, sfv.dtor, false);
ex = new CallExp(loc, ex);

dtorCalls ~= ex;
}
else
{
// _ArrayDtor((cast(S*)this.v.ptr)[0 .. n])

uinteger_t length = 1;
while (tv.ty == Tsarray)
{
length *= (cast(TypeSArray)tv).dim.toUInteger();
tv = tv.nextOf().toBasetype();
}
//if (n == 0)
// continue;

ex = new ThisExp(loc);
ex = new DotVarExp(loc, ex, sf);

// This is a hack so we can call destructors on const/immutable objects.
ex = new DotIdExp(loc, ex, Id.ptr);
ex = new CastExp(loc, ex, sdv.type.pointerTo());
if (stc & STC.safe)
stc = (stc & ~STC.safe) | STC.trusted;

ex = new SliceExp(loc, ex, new IntegerExp(loc, 0, Type.tsize_t),
new IntegerExp(loc, length, Type.tsize_t));
// Prevent redundant bounds check
(cast(SliceExp)ex).upperIsInBounds = true;
(cast(SliceExp)ex).lowerIsLessThanUpper = true;

ex = new CallExp(loc, new IdentifierExp(loc, Id._ArrayDtor), ex);

dtorCalls ~= ex;
}
}
fieldsToDestroy = [];

// aggregate the destructor calls
auto dtors = new Statements();
foreach_reverse(dc; dtorCalls)
{
dtors.push(new ExpStatement(loc, dc));
}

// put destructor calls in a `scope(failure)` block
postblitCalls.push(new OnScopeStatement(loc, TOK.onScopeFailure, new CompoundStatement(loc, dtors)));
}

// perform semantic on the member postblit in order to
// be able to aggregate it later on with the rest of the
// postblits
Expand Down Expand Up @@ -184,62 +263,17 @@ private extern (C++) FuncDeclaration buildPostBlit(StructDeclaration sd, Scope*
postblitCalls.push(new ExpStatement(loc, ex)); // combine in forward order

/* https://issues.dlang.org/show_bug.cgi?id=10972
* When the following field postblit calls fail,
* When subsequent field postblit calls fail,
* this field should be destructed for Exception Safety.
*/
if (!sdv.dtor)
continue;
sdv.dtor.functionSemantic();

tv = structField.type.toBasetype();
if (tv.ty == Tstruct)
{
// this.v.__xdtor()

ex = new ThisExp(loc);
ex = new DotVarExp(loc, ex, structField);

// This is a hack so we can call destructors on const/immutable objects.
ex = new AddrExp(loc, ex);
ex = new CastExp(loc, ex, structField.type.mutableOf().pointerTo());
ex = new PtrExp(loc, ex);
if (stc & STC.safe)
stc = (stc & ~STC.safe) | STC.trusted;

ex = new DotVarExp(loc, ex, sdv.dtor, false);
ex = new CallExp(loc, ex);
}
else
if (sdv.dtor)
{
// _ArrayDtor((cast(S*)this.v.ptr)[0 .. n])

uinteger_t length = 1;
while (tv.ty == Tsarray)
{
length *= (cast(TypeSArray)tv).dim.toUInteger();
tv = tv.nextOf().toBasetype();
}
//if (n == 0)
// continue;

ex = new ThisExp(loc);
ex = new DotVarExp(loc, ex, structField);

// This is a hack so we can call destructors on const/immutable objects.
ex = new DotIdExp(loc, ex, Id.ptr);
ex = new CastExp(loc, ex, sdv.type.pointerTo());
if (stc & STC.safe)
stc = (stc & ~STC.safe) | STC.trusted;

ex = new SliceExp(loc, ex, new IntegerExp(loc, 0, Type.tsize_t),
new IntegerExp(loc, length, Type.tsize_t));
// Prevent redundant bounds check
(cast(SliceExp)ex).upperIsInBounds = true;
(cast(SliceExp)ex).lowerIsLessThanUpper = true;
sdv.dtor.functionSemantic();

ex = new CallExp(loc, new IdentifierExp(loc, Id._ArrayDtor), ex);
// keep a list of fields that need to be destroyed in case
// of a future postblit failure
fieldsToDestroy ~= structField;
}
postblitCalls.push(new OnScopeStatement(loc, TOK.onScopeFailure, new ExpStatement(loc, ex)));
}

void checkShared()
Expand Down
16 changes: 16 additions & 0 deletions test/runnable/betterc.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,19 @@ extern (C) void test17605()
a = 1;
}

/*******************************************/
// https://issues.dlang.org/show_bug.cgi?id=18493

struct S18493
{
this(this) nothrow { } // Since this is attributed with `nothrow` there should be no error about using
// try-catch with -betterC
~this() { }
}

struct S18493_2
{
S18493 s1;
S18493 s2;
}

59 changes: 59 additions & 0 deletions test/runnable/xpostblit.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,67 @@ void test2() @safe @nogc nothrow
assert(Counter.cnt == 1);
}

/****************************************************************
This test is intended to verify the exception safety of field
postblits
*/
string trace = "";

struct FieldThrow
{
string name;
this(string n)
{
name = n;
}

bool throwExcept;
this(this)
{
if (throwExcept)
{
throw new Exception("");
}
}

~this() { trace ~= name ~ ".dtor"; }
}

struct S
{
auto f1 = FieldThrow("f1");
FieldThrow[2] f2f3= [FieldThrow("f2"), FieldThrow("f3")];
auto f4 = FieldThrow("f4");
}

void test3()
{
trace = "";

S s1;

// Cause `s1.f4`'s postblit to throw
s1.f4.throwExcept = true;

try
{
// `s`'s postblit will be a combination of `f1`, `f2f3`, and `f4`'s
// postblit in that order. However, `f4`'s postblit will throw,
// causing `s1.f2f3` and `s1.f1`'s destructors to execute in that
// order
S s2 = s1;
}
catch(Exception ex){ }

// Confirm the field destructors were called and were called in the
// corrrect order
assert(trace == "f3.dtor" ~ "f2.dtor" ~ "f1.dtor");
}
/****************************************************************************/

void main()
{
test1();
test2();
test3();
}