Skip to content
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

Jump to bottom

[stable] Fix misc. issues wrt. temporaries for -checkaction=context lowering #11005

Merged
merged 2 commits into from Apr 8, 2020
Merged

[stable] Fix misc. issues wrt. temporaries for -checkaction=context lowering #11005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b15b0c4
Fix misc. issues wrt. temporaries for -checkaction=context lowering
kinke Apr 4, 2020
f4dc07c
Handle mixin expressions as assert conditions
kinke Apr 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
84 changes: 43 additions & 41 deletions src/dmd/expressionsem.d
View file
Open in desktop
Expand Up @@ -5809,20 +5809,8 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
printf("AssertExp::semantic('%s')\n", exp.toChars());
}

// save expression as a string before any semantic expansion
// if -checkaction=context is enabled an no message exists
const generateMsg = !exp.msg && global.params.checkAction == CHECKACTION.context;
auto assertExpMsg = generateMsg ? exp.toChars() : null;

if (Expression ex = unaSemantic(exp, sc))
Copy link
Contributor Author

@kinke kinke Apr 4, 2020 •
edited

Choose a reason for hiding this comment

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

This early expression sema of exp.e1, the assert condition, was one of the problems. E.g.:

struct S
{
    int a;

    this(this) {}

    bool opEquals(S) @safe
    {
        return true;
    }
}

void foo()
{
    S a;
    assert(S() == a); // S().opEquals(a)
}

Here, an lvalue a is passed to S.opEquals(), which takes the arg by value. The early CallExp sema already lowers the argument expression a to a temporary, to something like S __copytmp = (__copytmp = a).__postblit(). That expression (instead of a) was previously stored as a further __assertOp temporary, and the original argument replaced by that temporary (without postblit).

Previous lowering:

assert(S(0).opEquals(((S __assertOp3 = (S __copytmp2 = (__copytmp2 = a).__postblit();) , __assertOp3 = __copytmp2;) , __assertOp3)), _d_assert_fail(S(0), __assertOp3));

New:

assert(S(0).opEquals(((S __copytmp2 = (__copytmp2 = a).__postblit();) , __copytmp2)), _d_assert_fail(S(0), a));

A related 2nd issue concerns rvalue arguments - the early CallExp sema detects that no temporary is needed (move; elide postblit and dtor in the caller); the rvalue was previously correctly stored as __assertOp temporary, but as it is now an lvalue (possibly accessed a 2nd time for the _d_assert_fail() call if the assertion fails), it must be copied + postblitted. This can be fixed by deferring the CallExp sema until the arguments have been replaced by required temporaries (and hence possibly promoted from rvalues to lvalues).

void rval()
{
    static S nonPure()
    {
        __gshared int counter;
        ++counter;
        return S(123);
    }
    assert(S() == nonPure());
}

Previous lowering:

assert(S(0).opEquals(((S __assertOp12 = nonPure();) , __assertOp12)), _d_assert_fail(S(0), __assertOp12));

New:

(S __assertOp11 = nonPure();) , assert(S(0).opEquals(((S __copytmp12 = (__copytmp12 = __assertOp11).__postblit();) , __copytmp12)), _d_assert_fail(S(0), __assertOp11));

{
result = ex;
return;
}
exp.e1 = resolveProperties(sc, exp.e1);
// BUG: see if we can do compile time elimination of the Assert
exp.e1 = exp.e1.optimize(WANTvalue);
exp.e1 = exp.e1.toBoolean(sc);
Expression temporariesPrefix;

if (generateMsg)
// no message - use assert expression as msg
Expand All @@ -5840,29 +5828,38 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
effects as in https://issues.dlang.org/show_bug.cgi?id=20114

Params:
op = an expression which may require a temporary and will be
replaced by `(auto tmp = op, tmp)` if necessary
op = an expression which may require a temporary (added to
`temporariesPrefix`: `auto tmp = op`) and will be replaced
by `tmp` if necessary

Returns: `op` or `tmp` for subsequent access to the possibly promoted operand
Returns: (possibly replaced) `op`
*/
Expression maybePromoteToTmp(ref Expression op)
{
op = op.expressionSemantic(sc);
op = resolveProperties(sc, op);
if (op.hasSideEffect)
{
const stc = STC.exptemp | (op.isLvalue() ? STC.ref_ : STC.rvalue);
const stc = op.isLvalue() ? STC.ref_ : 0;
auto tmp = copyToTemp(stc, "__assertOp", op);
tmp.dsymbolSemantic(sc);

auto decl = new DeclarationExp(op.loc, tmp);
auto var = new VarExp(op.loc, tmp);
auto comb = Expression.combine(decl, var);
op = comb.expressionSemantic(sc);
temporariesPrefix = Expression.combine(temporariesPrefix, decl);

return var;
op = new VarExp(op.loc, tmp);
op = op.expressionSemantic(sc);
}
return op;
}

// if the assert condition is a mixin expression, try to compile it
if (auto ce = exp.e1.isCompileExp())
{
if (auto e1 = compileIt(ce))
MoonlightSentinel marked this conversation as resolved.
Show resolved Hide resolved
exp.e1 = e1;
}

const tok = exp.e1.op;
bool isEqualsCallExpression;
if (tok == TOK.call)
Expand Down Expand Up @@ -5898,31 +5895,20 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
auto callExp = cast(CallExp) exp.e1;
auto args = callExp.arguments;

// template args
static immutable compMsg = "==";
Expression comp = new StringExp(loc, compMsg);
comp = comp.expressionSemantic(sc);
(*tiargs)[0] = comp;

// structs with opEquals get rewritten to a DotVarExp:
// a.opEquals(b)
// https://issues.dlang.org/show_bug.cgi?id=20100
if (args.length == 1)
{
auto dv = callExp.e1.isDotVarExp();
assert(dv);
(*tiargs)[1] = dv.e1.type;
(*tiargs)[2] = (*args)[0].type;

// runtime args
(*es)[0] = maybePromoteToTmp(dv.e1);
(*es)[1] = maybePromoteToTmp((*args)[0]);
}
else
{
(*tiargs)[1] = (*args)[0].type;
(*tiargs)[2] = (*args)[1].type;

// runtime args
(*es)[0] = maybePromoteToTmp((*args)[0]);
(*es)[1] = maybePromoteToTmp((*args)[1]);
Expand All @@ -5932,18 +5918,18 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
{
auto binExp = cast(EqualExp) exp.e1;

// template args
Expression comp = new StringExp(loc, Token.toString(exp.e1.op));
comp = comp.expressionSemantic(sc);
(*tiargs)[0] = comp;
(*tiargs)[1] = binExp.e1.type;
(*tiargs)[2] = binExp.e2.type;

// runtime args
(*es)[0] = maybePromoteToTmp(binExp.e1);
(*es)[1] = maybePromoteToTmp(binExp.e2);
}

// template args
Expression comp = new StringExp(loc, isEqualsCallExpression ? "==" : Token.toString(exp.e1.op));
comp = comp.expressionSemantic(sc);
(*tiargs)[0] = comp;
(*tiargs)[1] = (*es)[0].type;
(*tiargs)[2] = (*es)[1].type;

Expression __assertFail = new IdentifierExp(exp.loc, Id.empty);
auto assertFail = new DotIdExp(loc, __assertFail, Id.object);

Expand All @@ -5954,10 +5940,22 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
else
{
OutBuffer buf;
buf.printf("%s failed", assertExpMsg);
buf.printf("%s failed", exp.toChars());
exp.msg = new StringExp(Loc.initial, buf.extractSlice());
}
}

if (Expression ex = unaSemantic(exp, sc))
{
result = ex;
return;
}

exp.e1 = resolveProperties(sc, exp.e1);
// BUG: see if we can do compile time elimination of the Assert
exp.e1 = exp.e1.optimize(WANTvalue);
exp.e1 = exp.e1.toBoolean(sc);

if (exp.msg)
{
exp.msg = expressionSemantic(exp.msg, sc);
Expand Down Expand Up @@ -5999,8 +5997,12 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
return;
}
}

exp.type = Type.tvoid;
result = exp;

result = !temporariesPrefix
? exp
: Expression.combine(temporariesPrefix, exp).expressionSemantic(sc);
}

override void visit(DotIdExp exp)
Expand Down
62 changes: 58 additions & 4 deletions test/runnable/testassert.d
View file
Open in desktop
Expand Up @@ -75,6 +75,8 @@ void test20114()
assert(c == "1 != 0");
}

version (DigitalMars) version (Win64) version = DMD_Win64;

void test20375() @safe
{
static struct RefCounted
Expand All @@ -87,6 +89,7 @@ void test20375() @safe
}

static int instances;
static int postblits;

this(bool) @safe
{
Expand All @@ -96,10 +99,14 @@ void test20375() @safe
this(this) @safe
{
instances++;
postblits++;
}

~this() @safe
{
// make the dtor non-nothrow (we are tracking clean-ups during AssertError unwinding)
if (postblits > 100)
throw new Exception("");
assert(instances > 0);
instances--;
}
Expand All @@ -112,19 +119,54 @@ void test20375() @safe

{
auto a = RefCounted.create();
assert(a == RefCounted.create());
RefCounted.instances++; // we're about to construct an instance below, increment manually
assert(a == RefCounted()); // both operands are pure expressions => no temporaries
}

assert(RefCounted.instances == 0);
assert(RefCounted.postblits == 0);

{
auto a = RefCounted.create();
const msg = getMessage(assert(a != RefCounted.create()));
// assert(msg == "RefCounted() == RefCounted()"); // Currently not formatted
assert(msg == "assert(a != RefCounted.create()) failed");
assert(a == RefCounted.create()); // impure rhs is promoted to a temporary lvalue => copy for a.opEquals(rhs)
}

assert(RefCounted.instances == 0);
assert(RefCounted.postblits == 1);
RefCounted.postblits = 0;

{
const msg = getMessage(assert(RefCounted.create() != RefCounted.create())); // both operands promoted to temporary lvalues
assert(msg == "RefCounted() == RefCounted()");
}

version (DMD_Win64) // FIXME: temporaries apparently not destructed when unwinding via AssertError
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm mildly worried about a FIXME targeting stable, is tis problem EH, ABI, Codegen, something else?

Copy link
Contributor Author

@kinke kinke Apr 8, 2020 •
edited

Choose a reason for hiding this comment

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

I think many of these tests here are actually testing undefined behavior, as the lifetime of temporaries in statements throwing an AssertError is tracked, continuing in the tests after catching the AssertError, checking its message and checking that the temporaries have been properly destructed.

I'm unsure about the spec. Win64 with DMD seems to be the only platform eliding the cleanups, so I found it mildly worrying too, hence the comment, but at the same time I don't care about the potential DMD-specific codegen or druntime EH issue, as it's definitely unrelated to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

{
assert(RefCounted.instances >= 0 && RefCounted.instances <= 2);
RefCounted.instances = 0;
}
else
assert(RefCounted.instances == 0);
assert(RefCounted.postblits == 1);
RefCounted.postblits = 0;

static int numGetLvalImpureCalls = 0;
ref RefCounted getLvalImpure() @trusted
{
numGetLvalImpureCalls++;
__gshared lval = RefCounted(); // not incrementing RefCounted.instances
return lval;
}

{
const msg = getMessage(assert(getLvalImpure() != getLvalImpure())); // both operands promoted to ref temporaries
assert(msg == "RefCounted() == RefCounted()");
}

assert(numGetLvalImpureCalls == 2);
assert(RefCounted.instances == 0);
assert(RefCounted.postblits == 1);
RefCounted.postblits = 0;
}

string getMessage(T)(lazy T expr) @trusted
Expand All @@ -140,10 +182,22 @@ string getMessage(T)(lazy T expr) @trusted
}
}

void testMixinExpression() @safe
{
static struct S
{
bool opEquals(S) @safe { return true; }
}

const msg = getMessage(assert(mixin("S() != S()")));
assert(msg == "S() == S()");
}

void main()
{
test8765();
test9255();
test20114();
test20375();
testMixinExpression();
}