May 29, 2022 [Issue 17448] Move semantics cause memory corruption and cryptic bugs | ||||
|---|---|---|---|---|
| ||||
https://issues.dlang.org/show_bug.cgi?id=17448 Steven Schveighoffer <schveiguy@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |schveiguy@gmail.com --- Comment #43 from Steven Schveighoffer <schveiguy@gmail.com> --- I'm interested to hear back from the weka folks. As the original no longer is moved by the compiler, and also, DIP1014 is implemented in library code already, is there still a need to have any support for DIP1014 in the compiler? I tried to find a case where the compiler does a move, and after conversing with both Walter and Iain, I think there aren't any remaining cases. -- | ||||
May 30, 2022 [Issue 17448] Move semantics cause memory corruption and cryptic bugs | ||||
|---|---|---|---|---|
| ||||
https://issues.dlang.org/show_bug.cgi?id=17448 --- Comment #44 from Eyal <eyal@weka.io> --- Hi Steven, thanks for reaching out. This example still fails on newest dmd: struct S { S* addr; this(int dummy) { this.addr = &this; } static S create() { return S(1); } } unittest { auto s1 = S(1); assert(s1.addr == &s1); auto s2 = S.create(); assert(s2.addr == &s2); // <-- fails } While "@safe" disallows it, it also prevents all use of '&' here, so it is not really a practical option for such code. -- | ||||
May 30, 2022 [Issue 17448] Move semantics cause memory corruption and cryptic bugs | ||||
|---|---|---|---|---|
| ||||
https://issues.dlang.org/show_bug.cgi?id=17448 --- Comment #45 from Steven Schveighoffer <schveiguy@gmail.com> --- I'm assuming you are missing some extra machinery here. You aren't expecting D to automatically update your references for you, right? You at least need a postblit/copy ctor. -- | ||||
May 30, 2022 [Issue 17448] Move semantics cause memory corruption and cryptic bugs | ||||
|---|---|---|---|---|
| ||||
https://issues.dlang.org/show_bug.cgi?id=17448 --- Comment #46 from Steven Schveighoffer <schveiguy@gmail.com> --- e.g.: ```d struct S { S* addr; this(int dummy) { this.addr = &this; } this(ref const S other) { this.addr = &this; } static S create() { return S(1); } } ``` -- | ||||
May 30, 2022 [Issue 17448] Move semantics cause memory corruption and cryptic bugs | ||||
|---|---|---|---|---|
| ||||
https://issues.dlang.org/show_bug.cgi?id=17448 --- Comment #47 from Steven Schveighoffer <schveiguy@gmail.com> --- I should rephrase my query. currently, if you create an appropriately written `opPostMove`, then the D library calls that when it's doing a semantic move of your type (e.g. if you use `std.algorithm.move`) But I can't find a case where the *compiler* is doing a semantic move of the type. If there are no cases, then DIP1014 is already implemented, right? My query is really does anyone have any cases where the compiler performs the move? Re-parsing this bug report, I think the original request (provide a way to have an actual registration system that updates pointers when a move is done) is possible now. I feel this bug was closed for the wrong reason (i.e. toy examples aren't compiling any more), and I want to be sure it's actually possible now. The language *should* be more forthcoming on what constitutes a move, and what the compiler must do if it decides to move a struct. -- | ||||
May 30, 2022 [Issue 17448] Move semantics cause memory corruption and cryptic bugs | ||||
|---|---|---|---|---|
| ||||
https://issues.dlang.org/show_bug.cgi?id=17448 --- Comment #48 from Tomer Filiba (weka) <tomer@weka.io> --- Steven, can we agree that the following snippet is NOT okay? Regardless of @safe, let's leave it out of question. As a bonus, it this does not involve any library code/special moves/self references. And I used the latest dmd from run.dlang.io. ```d __gshared S* lastS = null; struct S { int x; this(int x) { this.x = x; lastS = &this; } } S f(int x) { return S(x); } void main() { S s1 = S(6); writeln("&s1=", &s1, " lastS=", lastS, " diff=", cast(void*)&s1 - cast(void*)lastS); // &s1=7FFD2262F468 lastS=7FFD2262F468 diff=0 S s2 = f(5); writeln("&s2=", &s2, " lastS=", lastS, " diff=", cast(void*)&s2 - cast(void*)lastS); // &s2=7FFD2262F46C lastS=7FFD2262F3F8 diff=116 } ``` * The first example, s1, is created using the ctor directly, which means there's no move and `&s1 == lastS`. * The second example, s2, return a struct via a function. Here we get an implicit move and lastS points to garbage, 116 bytes away from s2. * This means that if I use lastS, I'm either reading garbage from it, or corrupting my stack (i.e., assume I'm calling another function afterwards, which does `lastS.x = 17`) -- | ||||
May 30, 2022 [Issue 17448] Move semantics cause memory corruption and cryptic bugs | ||||
|---|---|---|---|---|
| ||||
https://issues.dlang.org/show_bug.cgi?id=17448 --- Comment #49 from Steven Schveighoffer <schveiguy@gmail.com> --- On discord, we worked out a case where the compiler is moving a struct and not calling any opPostMove or other features (copy constructor, postblit, etc): ```d import std.stdio; struct S{ S* ptr; this(int dummy) {ptr = &this;} this(this) {ptr = &this;} void opPostMove(const ref S oldLocation) nothrow { ptr = &this; } } void foo(S s){ assert(&s == s.ptr); // fails } S bar(){ auto s=S(1); auto t=S(1); if(true){ return t; } return s; } void main(){ foo(bar()); } ``` So there is still a need for the compiler to be aware of opPostMove. In this example, the return from bar cannot be NRVO optimized, and so a copy is made and the postblit called. Then in the call to foo, the return value of bar is moved into the parameter for foo. -- | ||||
May 30, 2022 [Issue 17448] Move semantics cause memory corruption and cryptic bugs | ||||
|---|---|---|---|---|
| ||||
https://issues.dlang.org/show_bug.cgi?id=17448 --- Comment #50 from Tomer Filiba (weka) <tomer@weka.io> --- Btw, I tried adding to S ``` @disable void opPostMove(const ref S oldLocation) nothrow {} ``` But it doesn't have any effect (problem compiles and produces the same issue). In C++, you can make types un-movable, which means that if there's need to move them, you'll get a compile time error. I would love to be able to replicate this behavior. -- | ||||
May 30, 2022 [Issue 17448] Move semantics cause memory corruption and cryptic bugs | ||||
|---|---|---|---|---|
| ||||
https://issues.dlang.org/show_bug.cgi?id=17448 --- Comment #51 from Steven Schveighoffer <schveiguy@gmail.com> --- (In reply to Tomer Filiba (weka) from comment #48) > Steven, can we agree that the following snippet is NOT okay? Regardless of @safe, let's leave it out of question. You are still not hooking copies or destructors. In this case, if I add a postblit to your struct like: ```d this(this) { lastS = &this; } ``` then it works as expected. In this case, the compiler is copying the S(x) instance to the return value, and then destroying the original, though I'm somewhat surprised NRVO didn't kick in here. In any case, I have found an answer to my question that the compiler *does* perform moves in certain situations, and it does not call opPostMove. -- | ||||
May 30, 2022 [Issue 17448] Move semantics cause memory corruption and cryptic bugs | ||||
|---|---|---|---|---|
| ||||
https://issues.dlang.org/show_bug.cgi?id=17448 --- Comment #52 from Steven Schveighoffer <schveiguy@gmail.com> --- (In reply to Steven Schveighoffer from comment #51) > then it works as expected. In this case, the compiler is copying the S(x) instance to the return value, and then destroying the original, though I'm somewhat surprised NRVO didn't kick in here. Indeed, it's enough to disable postblit, and then NRVO kicks in. So the compiler prefers to move vs. using NRVO when nobody cares about the movement/copy. And then it becomes apparent that the compiler must obey `@disable opPostMove` if it is going to make decisions like this! -- | ||||
Copyright © 1999-2021 by the D Language Foundation
Permalink
Reply