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

kinke · 2020-04-04T18:42:02Z

No description provided.

kinke · 2020-04-04T19:18:10Z

src/dmd/expressionsem.d

        const generateMsg = !exp.msg && global.params.checkAction == CHECKACTION.context;
-        auto assertExpMsg = generateMsg ? exp.toChars() : null;
-
-        if (Expression ex = unaSemantic(exp, sc))


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));

This is blocking dlang/dmd#11005.

…ields This is blocking dlang/dmd#11005.

WalterBright · 2020-04-05T09:43:56Z

Fix misc. issues

Please do ONE issue per PR.

kinke · 2020-04-05T14:15:00Z

Deferring the CallExp sema fixes multiple issues at once. - I've been testing this with LDC, as I don't have a DMD dev environment set up, and I can't reproduce (on Windows) the remaining failure in

dmd/test/runnable/testassert.d

Line 126 in ed05c88

assert(RefCounted.instances == 0);

(apparently 1 instance is still alive with DMD)

src/dmd/expressionsem.d

kinke · 2020-04-05T18:44:03Z

The mentioned DMD failure can be worked around by making RefCounted.~this() non-nothrow; I guess that's an unrelated other bug (invalid dtor elision? or possibly allowed because the stack is unwound via an AssertError?).

Edit: Still not enough for Win64.

kinke · 2020-04-05T19:48:11Z

druntime would need this:

diff --git a/test/exceptions/src/assert_fail.d b/test/exceptions/src/assert_fail.d
index 3e21ef64..c31ad4ee 100644
--- a/test/exceptions/src/assert_fail.d
+++ b/test/exceptions/src/assert_fail.d
@@ -109,7 +109,7 @@ void testToString()()
         }
     }
 
-    test!"!="(Overloaded(), Overloaded(), "Const is Const");
+    test!"!="(Overloaded(), Overloaded(), "Const == Const");
 }
 
 
@@ -129,8 +129,8 @@ void testStruct()()
 {
     struct S { int s; }
     struct T { T[] t; }
-    test(S(0), S(1), "S(0) !is S(1)");
-    test(T([T(null)]), T(null), "[T([])] != []");
+    test(S(0), S(1), "S(0) != S(1)");
+    test(T([T(null)]), T(null), "T([T([])]) != T([])");
 
     // https://issues.dlang.org/show_bug.cgi?id=20323
     static struct NoCopy
@@ -139,7 +139,7 @@ void testStruct()()
     }
 
     NoCopy n;
-    test(assert(n != n), "NoCopy() is NoCopy()");
+    test(assert(n != n), "NoCopy() == NoCopy()");
 }
 
 void testAA()()
@@ -161,13 +161,13 @@ void testVoidArray()()
 {
     assert([] is null);
     assert(null is null);
-    test([1], null, "[1] != []");
-    test("s", null, `"s" != ""`);
-    test(['c'], null, `"c" != ""`);
+    test([1], null, "[1] != `null`");
+    test("s", null, "\"s\" != `null`");
+    test(['c'], null, "\"c\" != `null`");
     test!"!="(null, null, "`null` == `null`");
 
     const void[] chunk = [byte(1), byte(2), byte(3)];
-    test(chunk, null, "[1, 2, 3] != []");
+    test(chunk, null, "[1, 2, 3] != `null`");
 }
 
 void testTemporary()
@@ -177,7 +177,7 @@ void testTemporary()
         ~this() @system {}
     }
 
-    test(assert(Bad() != Bad()), "Bad() is Bad()");
+    test(assert(Bad() != Bad()), "Bad() == Bad()");
 }
 
 void testEnum()

MoonlightSentinel · 2020-04-05T20:15:44Z

druntime would need this: [...]

The first part looks nice because it retains the original types and operators. But the second part looks kindof bad now, especially when comparing strings and null.
Maybe d_assert_fail could detect if either operator is typeof(null) and apply the type of the other operand?

kinke · 2020-04-06T01:44:32Z

I don't care how null could be handled more elegantly (i.e., the druntime side of this), but as most of the time, it's a pain to get things done here due to the druntime/compiler entanglement.

MoonlightSentinel · 2020-04-06T13:37:07Z

That can be handled in a later PR, was just thinking about different approaches to retain some of the current behaviour.

The current test situation for -checkaction=context seems weird. I think it would be beneficial (esp. for your PR) to replace test/exceptions/src/assert_fail.d as follows:

Tests checking correct formatting / attribute interference become unittest explicitly calling d_assert_fail
Lowering test are moved into dmd's test suite.

What do you think?

kinke · 2020-04-06T16:10:31Z

Yes, that would be exactly it. I was planning to finish this for 2.091.1 / LDC 1.21.0 final, but that's no longer an option. ;)

MoonlightSentinel · 2020-04-06T16:17:36Z

Guess i'll start to apply the aforementioned changes. Whats the schedule for LDC 1.21.0 final?

kinke · 2020-04-06T16:23:58Z

Thx, help is greatly appreciated. - The usual schedule would be something like max 1 week after the 2.091.1 release, i.e., soon, but it's not set in stone (and if I really wanted to, I could of course make the LDC tests pass and handle it here upstream in parallel).

This test should only check the Druntime component of `-checkaction=context`, i.e. that _d_assert_fail yields an appropriate assert message. Using `assert` directly linked this test to the lowering details and made changes changes quite difficult because one cannot make changes for DMD + Druntime in a single PR (see e.g. dlang/dmd#11005).

This test should only check the druntime component of `-checkaction=context`, i.e. that `_d_assert_fail` yields an appropriate assert message. Using `assert` directly linked this test to the lowering details and made changes changes quite difficult because one cannot make changes to dmd and druntime in a single PR (see e.g. dlang/dmd#11005).

src/dmd/expressionsem.d

dlang-bot · 2020-04-07T16:58:20Z

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

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

dub run digger -- build "stable + dmd#11005"

test/runnable/testassert.d

@safe

The early expression sema of `exp.e1`, the assert condition, was a major problem. 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 lowering: 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 lowering: (S __assertOp11 = nonPure();) , assert(S(0).opEquals(((S __copytmp12 = (__copytmp12 = __assertOp11).__postblit();) , __copytmp12)), _d_assert_fail(S(0), __assertOp11));

For druntime's test/exceptions/src/assert_fail.d.

thewilsonator

Otherwise looks fine.

thewilsonator · 2020-04-08T00:51:15Z

test/runnable/testassert.d

+        assert(msg == "RefCounted() == RefCounted()");
+    }
+
+    version (DMD_Win64) // FIXME: temporaries apparently not destructed when unwinding via AssertError


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

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.

Fair enough.

This test should only check the druntime component of `-checkaction=context`, i.e. that `_d_assert_fail` yields an appropriate assert message. Using `assert` directly linked this test to the lowering details and made changes changes quite difficult because one cannot make changes to dmd and druntime in a single PR (see e.g. dlang/dmd#11005).

kinke force-pushed the context branch from 73bd1c6 to 3cfa5a1 Compare April 4, 2020 18:53

kinke commented Apr 4, 2020

View reviewed changes

kinke force-pushed the context branch from 3cfa5a1 to 16a22f5 Compare April 4, 2020 21:03

kinke mentioned this pull request Apr 4, 2020

[stable] core.internal.string: Make non-mutable TempStringNoAlloc usable dlang/druntime#3017

Merged

kinke added a commit to kinke/druntime that referenced this pull request Apr 4, 2020

core.internal.string: Make non-mutable TempStringNoAlloc usable

9730d22

This is blocking dlang/dmd#11005.

kinke force-pushed the context branch from 16a22f5 to 881c7ba Compare April 5, 2020 00:59

kinke added a commit to kinke/druntime that referenced this pull request Apr 5, 2020

core.internal.dassert: Fix miniFormat for structs with non-copyable f…

68972cc

…ields This is blocking dlang/dmd#11005.

kinke mentioned this pull request Apr 5, 2020

[stable] core.internal.dassert: Fix miniFormat for structs with non-copyable fields dlang/druntime#3018

Merged

kinke force-pushed the context branch 2 times, most recently from 62d7d84 to ed05c88 Compare April 5, 2020 13:24

MoonlightSentinel reviewed Apr 5, 2020

View reviewed changes

src/dmd/expressionsem.d Outdated Show resolved Hide resolved

This was referenced Apr 5, 2020

Fix Issue 20353 - -checkaction=context does not work well with const … #10535

Closed

Fix Issue 20581 - DIP1000 wrongly flags hidden ref temporary #10787

Closed

kinke closed this Apr 5, 2020

kinke reopened this Apr 5, 2020

kinke force-pushed the context branch from dc63432 to 6f92420 Compare April 5, 2020 19:56

MoonlightSentinel mentioned this pull request Apr 6, 2020

Explicitly call _d_assert_fail in test/exceptions/assert_fail.d dlang/druntime#3019

Merged

kinke force-pushed the context branch 3 times, most recently from 818447c to 3a62577 Compare April 7, 2020 13:15

MoonlightSentinel reviewed Apr 7, 2020

View reviewed changes

src/dmd/expressionsem.d Show resolved Hide resolved

kinke force-pushed the context branch 4 times, most recently from 6272c84 to a25884a Compare April 7, 2020 16:34

kinke marked this pull request as ready for review April 7, 2020 16:58

kinke changed the title ~~Fix misc. issues wrt. temporaries for -checkaction=context lowering~~ [stable] Fix misc. issues wrt. temporaries for -checkaction=context lowering Apr 7, 2020

MoonlightSentinel reviewed Apr 7, 2020

View reviewed changes

test/runnable/testassert.d Outdated Show resolved Hide resolved

kinke added 2 commits April 8, 2020 01:06

Handle mixin expressions as assert conditions

f4dc07c

For druntime's test/exceptions/src/assert_fail.d.

kinke force-pushed the context branch from a25884a to f4dc07c Compare April 7, 2020 23:06

thewilsonator approved these changes Apr 8, 2020

View reviewed changes

thewilsonator added the auto-merge label Apr 8, 2020

dlang-bot merged commit cbe2eee into dlang:stable Apr 8, 2020

kinke deleted the context branch April 8, 2020 01:27

MoonlightSentinel mentioned this pull request Apr 20, 2020

Extend checkaction=context for boolean convertible expressions #11051

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kinke commented Apr 4, 2020

kinke Apr 4, 2020 •

edited

WalterBright commented Apr 5, 2020

kinke commented Apr 5, 2020 •

edited

kinke commented Apr 5, 2020 •

edited

kinke commented Apr 5, 2020

MoonlightSentinel commented Apr 5, 2020

kinke commented Apr 6, 2020 •

edited

MoonlightSentinel commented Apr 6, 2020

kinke commented Apr 6, 2020

MoonlightSentinel commented Apr 6, 2020

kinke commented Apr 6, 2020

dlang-bot commented Apr 7, 2020

thewilsonator left a comment

thewilsonator Apr 8, 2020

kinke Apr 8, 2020 •

edited

thewilsonator Apr 8, 2020

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

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

Conversation

kinke commented Apr 4, 2020

kinke Apr 4, 2020 • edited

Choose a reason for hiding this comment

WalterBright commented Apr 5, 2020

kinke commented Apr 5, 2020 • edited

kinke commented Apr 5, 2020 • edited

kinke commented Apr 5, 2020

MoonlightSentinel commented Apr 5, 2020

kinke commented Apr 6, 2020 • edited

MoonlightSentinel commented Apr 6, 2020

kinke commented Apr 6, 2020

MoonlightSentinel commented Apr 6, 2020

kinke commented Apr 6, 2020

dlang-bot commented Apr 7, 2020

Bugzilla references

Testing this PR locally

thewilsonator left a comment

Choose a reason for hiding this comment

thewilsonator Apr 8, 2020

Choose a reason for hiding this comment

kinke Apr 8, 2020 • edited

Choose a reason for hiding this comment

thewilsonator Apr 8, 2020

Choose a reason for hiding this comment

kinke Apr 4, 2020 •

edited

kinke commented Apr 5, 2020 •

edited

kinke commented Apr 5, 2020 •

edited

kinke commented Apr 6, 2020 •

edited

kinke Apr 8, 2020 •

edited