From a9ee4ce6c0e4e0e4981a367470e8645f2cb85271 Mon Sep 17 00:00:00 2001 From: Walter Bright Date: Fri, 14 Sep 2018 15:32:53 -0700 Subject: [PATCH] fix Issue 14246 - RAII - proper destruction of partially constructed objects/structs --- changelog/reboot14246.dd | 13 +++++++++ src/dmd/aggregate.d | 1 + src/dmd/aggregate.h | 1 + src/dmd/cli.d | 2 ++ src/dmd/clone.d | 1 + src/dmd/globals.d | 2 ++ src/dmd/globals.h | 2 ++ src/dmd/semantic3.d | 62 ++++++++++++++++++++++++++++++++++++++++ src/dmd/statement.d | 12 ++++++++ test/runnable/sdtor.d | 50 ++++++++++++++++++++++++++++++++ 10 files changed, 146 insertions(+) create mode 100644 changelog/reboot14246.dd diff --git a/changelog/reboot14246.dd b/changelog/reboot14246.dd new file mode 100644 index 000000000000..3a88fbcaa430 --- /dev/null +++ b/changelog/reboot14246.dd @@ -0,0 +1,13 @@ +fix Issue 14246 - RAII - proper destruction of partically constructed objects + +Since destructors for object fields can now get called by the constructor for that +object if the constructor fails, the attributes of the destructors must be covariant +with the attributes for the constructor. + +I.e. if the constructor is marked @safe, the fields' destructors must also be @safe +or @trusted. + +Since this behavior was never checked before, this fix causes breakage of existing +code. Hence, enabling this behavior is behind the new `transition=dtorfields` compiler +switch. It will eventually become the default behavior, so it is recommended to +fix any of these destructor attribute issues. diff --git a/src/dmd/aggregate.d b/src/dmd/aggregate.d index 3e2d1e4b4bfb..586b57c71086 100644 --- a/src/dmd/aggregate.d +++ b/src/dmd/aggregate.d @@ -115,6 +115,7 @@ extern (C++) abstract class AggregateDeclaration : ScopeDsymbol DtorDeclaration dtor; // aggregate destructor DtorDeclaration primaryDtor; // non-deleting C++ destructor, same as dtor for D DtorDeclaration tidtor; // aggregate destructor used in TypeInfo (must have extern(D) ABI) + FuncDeclaration fieldDtor; // aggregate destructor for just the fields Expression getRTInfo; // pointer to GC info generated by object.RTInfo(this) diff --git a/src/dmd/aggregate.h b/src/dmd/aggregate.h index ff4f34496940..f8bb2b1419ed 100644 --- a/src/dmd/aggregate.h +++ b/src/dmd/aggregate.h @@ -127,6 +127,7 @@ class AggregateDeclaration : public ScopeDsymbol DtorDeclaration *dtor; // aggregate destructor DtorDeclaration *primaryDtor; // non-deleting C++ destructor, same as dtor for D DtorDeclaration *tidtor; // aggregate destructor used in TypeInfo (must have extern(D) ABI) + FuncDeclaration *fieldDtor; // aggregate destructor for just the fields Expression *getRTInfo; // pointer to GC info generated by object.RTInfo(this) diff --git a/src/dmd/cli.d b/src/dmd/cli.d index 7131f68ef581..e9c66437b937 100644 --- a/src/dmd/cli.d +++ b/src/dmd/cli.d @@ -605,6 +605,8 @@ dmd -cov -unittest myprog.d "list all non-mutable fields which occupy an object instance"), Transition("10378", "import", "bug10378", "revert to single phase name lookup"), + Transition("14246", "dtorfields", "dtorFields", + "destruct fields of partially constructed objects"), Transition(null, "checkimports", "check10378", "give deprecation messages about 10378 anomalies"), Transition("14488", "complex", "vcomplex", diff --git a/src/dmd/clone.d b/src/dmd/clone.d index b396554f0e6a..c6cd909b493d 100644 --- a/src/dmd/clone.d +++ b/src/dmd/clone.d @@ -963,6 +963,7 @@ extern (C++) DtorDeclaration buildDtor(AggregateDeclaration ad, Scope* sc) ad.dtors.shift(dd); ad.members.push(dd); dd.dsymbolSemantic(sc); + ad.fieldDtor = dd; } } diff --git a/src/dmd/globals.d b/src/dmd/globals.d index f4b2c162afb2..2149b98b0fcc 100644 --- a/src/dmd/globals.d +++ b/src/dmd/globals.d @@ -151,6 +151,8 @@ struct Param // https://issues.dlang.org/show_bug.cgi?id=16997 bool vsafe; // use enhanced @safe checking bool ehnogc; // use @nogc exception handling + bool dtorFields; // destruct fields of partially constructed objects + // https://issues.dlang.org/show_bug.cgi?id=14246 /** The --transition=safe switch should only be used to show code with * silent semantics changes related to @safe improvements. It should not be * used to hide a feature that will have to go through deprecate-then-error diff --git a/src/dmd/globals.h b/src/dmd/globals.h index ef0cc2d43ba5..22652d33feb6 100644 --- a/src/dmd/globals.h +++ b/src/dmd/globals.h @@ -124,6 +124,8 @@ struct Param // https://issues.dlang.org/show_bug.cgi?id=16997 bool vsafe; // use enhanced @safe checking bool ehnogc; // use @nogc exception handling + bool dtorFields; // destruct fields of partially constructed objects + // https://issues.dlang.org/show_bug.cgi?id=14246 bool showGaggedErrors; // print gagged errors anyway bool manual; // open browser on compiler manual bool usage; // print usage and exit diff --git a/src/dmd/semantic3.d b/src/dmd/semantic3.d index 06caddbb2714..4ef835a4ef96 100644 --- a/src/dmd/semantic3.d +++ b/src/dmd/semantic3.d @@ -1255,6 +1255,68 @@ private extern(C++) final class Semantic3Visitor : Visitor //fflush(stdout); } + override void visit(CtorDeclaration ctor) + { + //printf("CtorDeclaration()\n%s\n", ctor.fbody.toChars()); + if (ctor.semanticRun >= PASS.semantic3) + return; + + /* If any of the fields of the aggregate have a destructor, add + * scope (failure) { this.fieldDtor(); } + * as the first statement. It is not necessary to add it after + * each initialization of a field, because destruction of .init constructed + * structs should be benign. + * https://issues.dlang.org/show_bug.cgi?id=14246 + */ + AggregateDeclaration ad = ctor.toParent2().isAggregateDeclaration(); + if (ad && ad.fieldDtor && global.params.dtorFields) + { + /* Generate: + * this.fieldDtor() + */ + Expression e = new ThisExp(ctor.loc); + e.type = ad.type.mutableOf(); + e = new DotVarExp(ctor.loc, e, ad.fieldDtor, false); + e = new CallExp(ctor.loc, e); + auto sexp = new ExpStatement(ctor.loc, e); + auto ss = new ScopeStatement(ctor.loc, sexp, ctor.loc); + + version (all) + { + /* Generate: + * try { ctor.fbody; } + * catch (Exception __o) + * { this.fieldDtor(); throw __o; } + * This differs from the alternate scope(failure) version in that an Exception + * is caught rather than a Throwable. This enables the optimization whereby + * the try-catch can be removed if ctor.fbody is nothrow. (nothrow only + * applies to Exception.) + */ + Identifier id = Identifier.generateId("__o"); + auto ts = new ThrowStatement(ctor.loc, new IdentifierExp(ctor.loc, id)); + auto handler = new CompoundStatement(ctor.loc, ss, ts); + + auto catches = new Catches(); + auto ctch = new Catch(ctor.loc, getException(), id, handler); + catches.push(ctch); + + ctor.fbody = new TryCatchStatement(ctor.loc, ctor.fbody, catches); + } + else + { + /* Generate: + * scope (failure) { this.fieldDtor(); } + * Hopefully we can use this version someday when scope(failure) catches + * Exception instead of Throwable. + */ + auto s = new OnScopeStatement(ctor.loc, TOK.onScopeFailure, ss); + ctor.fbody = new CompoundStatement(ctor.loc, s, ctor.fbody); + } + } + visit(cast(FuncDeclaration)ctor); + } + + override void visit(Nspace ns) { if (ns.semanticRun >= PASS.semantic3) diff --git a/src/dmd/statement.d b/src/dmd/statement.d index af4322093b92..59a32842c71d 100644 --- a/src/dmd/statement.d +++ b/src/dmd/statement.d @@ -60,6 +60,18 @@ TypeIdentifier getThrowable() return tid; } +/** + * Returns: + * TypeIdentifier corresponding to `object.Exception` + */ +TypeIdentifier getException() +{ + auto tid = new TypeIdentifier(Loc.initial, Id.empty); + tid.addIdent(Id.object); + tid.addIdent(Id.Exception); + return tid; +} + /*********************************************************** * Specification: http://dlang.org/spec/statement.html diff --git a/test/runnable/sdtor.d b/test/runnable/sdtor.d index 0ab5a2a13292..a5e3a08c535f 100644 --- a/test/runnable/sdtor.d +++ b/test/runnable/sdtor.d @@ -1,4 +1,5 @@ // PERMUTE_ARGS: -unittest -O -release -inline -fPIC -g +// REQUIRED_ARGS: -transition=dtorfields import core.vararg; @@ -4229,6 +4230,38 @@ int test14860() } static assert(test14860()); +/**********************************/ +// https://issues.dlang.org/show_bug.cgi?id=14246 + +struct A14246 { + int a = 3; + static string s; + this( int var ) { printf("A()\n"); a += var; s ~= "a"; } + + ~this() { printf("~A()\n"); s ~= "b"; } +} + +struct B14246 { + int i; + A14246 a; + + this( int var ) { + A14246.s ~= "c"; + a = A14246(var+1); + throw new Exception("An exception"); + } +} + +void test14246() { + try { + auto b = B14246(2); + } catch( Exception ex ) { + printf("Caught ex\n"); + A14246.s ~= "d"; + } + assert(A14246.s == "cabd"); +} + /**********************************/ // 14696 @@ -4563,6 +4596,22 @@ void test18045() nothrow /**********************************/ +struct S66 +{ + ~this() { } +} + +nothrow void notthrow() { } + +class C66 +{ + S66 s; + + this() nothrow { notthrow(); } +} + +/**********************************/ + int main() { test1(); @@ -4687,6 +4736,7 @@ int main() test14815(); test16197(); test14860(); + test14246(); test14696(); test14838(); test63();