Bug 21357 – [REG2.093] postblit aliases old and new struct pointers

Testcase: ``` struct BatchState { int[10] arr; // change size to 1 to remove the bug BatchState copy() { auto ret = BatchState(arr); arr[0] += 1; return ret; } } struct GrayArea { BatchState low; this(this) { low = low.copy; } } void main() { GrayArea a; a.low.arr[0] = 1; GrayArea b; b.low.arr[0] = 4; b = a; // calls the postblit import std.stdio; writeln(a.low.arr[0]); writeln(b.low.arr[0]); } ``` Old output: ❯ ~/dlang/dmd-2.092.1/linux/bin64/dmd -run bug.d 1 1 2.093 output: ❯ ~/dlang/dmd-2.093.1/linux/bin64/dmd -run bug.d 1 2 The problem is the following: 1. BatchState is a large struct, which means that `BatchState.copy()` is passed a pointer to a caller-allocated struct to store its return value (instead of returning a value through a register). 2. The call `low = low.copy` in `this(this)` _should_ be creating a temporary BatchState struct on the stack and pass a pointer to that to `BatchState.copy()` so it can store the return value there. That is, the call should be transformed to something like: `BatchState temp; BatchState.copy(&temp, &low); low = temp;`. Instead what happens since 2.093 is that the call becomes: `BatchState.copy(&low, &low);`. This is a very serious bug. Common code like `this(this) { member = member.dup; }` is now utterly broken.

(In reply to johanengelen from comment #0) > The problem is the following: > 1. BatchState is a large struct, which means that `BatchState.copy()` is > passed a pointer to a caller-allocated struct to store its return value > (instead of returning a value through a register). I wouldn't count that as a problem, returning large structs is always done using a call slot optimization. It would be more likely whether the callee copies into it or uses the return address directly. The latter elides a copy, but I don't initially see any reason BatchState should elide a copy (it is POD afterall). Is there an identified regressing commit yet? Or should I trigger a bisect.

Caused by: commit 68275da1a9785ad0bf15782da6502948171271bd Author: Martin Kinkelin <[email protected]> Date: Wed May 27 00:58:09 2020 +0200 Fix Issue 11292 - Cannot re-initialize a const field in postblit

(In reply to Boris Carvajal from comment #2) > Caused by: > > commit 68275da1a9785ad0bf15782da6502948171271bd > Author: Martin Kinkelin <[email protected]> > Date: Wed May 27 00:58:09 2020 +0200 > > Fix Issue 11292 - Cannot re-initialize a const field in postblit Confirmed. The change alters the AssignExp `this.low = this.low.copy()` into a ConstructExp. This is wrong, postblits don't initialize new memory.

@ibuclaw updated dlang/dmd pull request #11921 "Revert "Fix Issue 11292 - Cannot re-initialize a const field in postblit"" fixing this issue: - fix Issue 21357 - [REG2.093] postblit aliases old and new struct pointers https://github.com/dlang/dmd/pull/11921

(In reply to Iain Buclaw from comment #1) > (In reply to johanengelen from comment #0) > > The problem is the following: > > 1. BatchState is a large struct, which means that `BatchState.copy()` is > > passed a pointer to a caller-allocated struct to store its return value > > (instead of returning a value through a register). > > I wouldn't count that as a problem, Neither do I. The problem is 2 indeed. -Johan

dlang/dmd pull request #11921 "Revert "Fix Issue 11292 - Cannot re-initialize a const field in postblit"" was merged into master: - 14b87dbb0f9dfef258bf13041fcacd9dc2d7ac30 by Iain Buclaw: fix Issue 21357 - [REG2.093] postblit aliases old and new struct pointers https://github.com/dlang/dmd/pull/11921