www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - Struct delegate access corruption

reply z <z z.com> writes:
So i've upgraded one of my structs to use the more flexible 
delegates instead of function pointers but when the member 
function tries to access the struct's members, the contents are 
random and the program fails.

i've isolated the problem by adding writefln calls before calling 
the delegate and inside the delegate(the functions are defined in 
the struct as member functions, the delegate itself is set in the 
constructor) :

In the code that uses the delegate :
writefln!"test %s"(a, &a);
T b = a.d();//the delegate
While in the most used delegate :
writefln!"test2 %s %s"(this, &this);
The contents and pointers don't match(they're random, full of 0, -nan, -inf and other invalid values), are they(delegates) supposed to be used like this? Big thanks
Feb 17
next sibling parent Steven Schveighoffer <schveiguy gmail.com> writes:
On 2/17/21 10:38 AM, z wrote:
 So i've upgraded one of my structs to use the more flexible delegates 
 instead of function pointers but when the member function tries to 
 access the struct's members, the contents are random and the program fails.
 
 i've isolated the problem by adding writefln calls before calling the 
 delegate and inside the delegate(the functions are defined in the struct 
 as member functions, the delegate itself is set in the constructor) :
 
 In the code that uses the delegate :
 writefln!"test %s"(a, &a);
 T b = a.d();//the delegate
While in the most used delegate :
 writefln!"test2 %s %s"(this, &this);
The contents and pointers don't match(they're random, full of 0, -nan, -inf and other invalid values), are they(delegates) supposed to be used like this?
With structs and delegates, you have to be careful because structs are copied easily, and using a delegate on a struct that is no longer in scope is going to lead to memory corruption. In order to properly ensure delegates are pointing to valid data, make sure the struct is still not moved or overwritten, or it is allocated on the heap. -Steve
Feb 17
prev sibling parent reply "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Wed, Feb 17, 2021 at 03:38:00PM +0000, z via Digitalmars-d-learn wrote:
 So i've upgraded one of my structs to use the more flexible delegates
 instead of function pointers but when the member function tries to
 access the struct's members, the contents are random and the program
 fails.
You're likely running into the struct self-reference problem. Keep in mind that in D, structs are what Andrei calls "glorified ints", i.e., they are value types that get freely copied around and passed around in registers. Meaning that the address of a struct is likely to change (and change a lot as it gets passed around), unless you explicitly allocated it on the heap. So if you have any pointers to the struct (including implicit pointers like delegate context pointers) stored inside itself, they are highly likely to become dangling pointers as the struct gets copied to another place and the old copy goes out of scope. I.e., the following is NOT a good idea: struct S { void delegate() member; this() { member = &this.impl; } private void impl(); } because a pointer to S is stored inside S itself, so as soon as S gets copied or moved, the delegate context pointer is no longer valid (or else points to a different copy of the struct than the one it will be called from). If you need to store references to an aggregate inside itself, you should be using a class instead. Or be absolutely sure you allocate the struct on the heap with `new`, AND never pass it around except by reference (using pointers). T -- Almost all proofs have bugs, but almost all theorems are true. -- Paul Pedersen
Feb 17
parent reply tsbockman <thomas.bockman gmail.com> writes:
On Wednesday, 17 February 2021 at 17:45:01 UTC, H. S. Teoh wrote:
 I.e., the following is NOT a good idea:

 	struct S {
 		void delegate() member;
 		this() {
 			member = &this.impl;
 		}
 		private void impl();
 	}

 because a pointer to S is stored inside S itself, so as soon as 
 S gets copied or moved, the delegate context pointer is no 
 longer valid (or else points to a different copy of the struct 
 than the one it will be called from).
A copy constructor and opAssign can be used to update pointers that are relative to &this: https://dlang.org/spec/struct.html#struct-copy-constructor // The following program prints 9 with the copy constructor, or 7 if it is commented out: module app; import std.stdio; struct Foo { int[2] things; private int* ptr; this(const(int[2]) things...) inout pure trusted nothrow nogc { this.things = things; ptr = &(this.things[0]); } // Copy constructor: this(scope ref const(typeof(this)) that) inout pure trusted nothrow nogc { this.things = that.things; this.ptr = this.things.ptr + (that.ptr - that.things.ptr); } // Defining a matching opAssign is a good idea, as well: ref typeof(this) opAssign(scope ref const(typeof(this)) that) return pure trusted nothrow nogc { __ctor(that); return this; } void choose(const(int) x) pure trusted nothrow nogc { ptr = &(things[x]); } property ref inout(int) chosen() return inout pure safe nothrow nogc { return *ptr; } } void main() { auto foo = Foo(3, 7); foo.choose(1); Foo bar = foo; bar.things[1] = 9; writeln(bar.chosen); }
Feb 17
parent reply Paul Backus <snarwin gmail.com> writes:
On Wednesday, 17 February 2021 at 19:42:00 UTC, tsbockman wrote:
 On Wednesday, 17 February 2021 at 17:45:01 UTC, H. S. Teoh 
 wrote:
 I.e., the following is NOT a good idea:

 	struct S {
 		void delegate() member;
 		this() {
 			member = &this.impl;
 		}
 		private void impl();
 	}

 because a pointer to S is stored inside S itself, so as soon 
 as S gets copied or moved, the delegate context pointer is no 
 longer valid (or else points to a different copy of the struct 
 than the one it will be called from).
A copy constructor and opAssign can be used to update pointers that are relative to &this: https://dlang.org/spec/struct.html#struct-copy-constructor
Unfortunately this is not enough, because the compiler is free to implicitly move struct instances in memory any time it wants to. See the bug report below for more details: https://issues.dlang.org/show_bug.cgi?id=17448 Until D gets move constructors, structs with interior pointers should be avoided.
Feb 17
parent reply tsbockman <thomas.bockman gmail.com> writes:
On Wednesday, 17 February 2021 at 20:18:53 UTC, Paul Backus wrote:
 On Wednesday, 17 February 2021 at 19:42:00 UTC, tsbockman wrote:
 A copy constructor and opAssign can be used to update pointers 
 that are relative to &this:
     https://dlang.org/spec/struct.html#struct-copy-constructor
Unfortunately this is not enough, because the compiler is free to implicitly move struct instances in memory any time it wants to. See the bug report below for more details: https://issues.dlang.org/show_bug.cgi?id=17448 Until D gets move constructors, structs with interior pointers should be avoided.
That bug is about postblits, this(this), not copy constructors: this(ref typeof(this)). Copy constructors were added to the language specifically to fix those sort of problems. From the spec: https://dlang.org/spec/struct.html#struct-postblit
 WARNING: The postblit is considered legacy and is not 
 recommended for new code. Code should use copy constructors 
 defined in the previous section. For backward compatibility 
 reasons, a struct that explicitly defines both a copy 
 constructor and a postblit will only use the postblit for 
 implicit copying. However, if the postblit is disabled, the 
 copy constructor will be used. If a struct defines a copy 
 constructor (user-defined or generated) and has fields that 
 define postblits, a deprecation will be issued, informing that 
 the postblit will have priority over the copy constructor.
question to the issue report asking for clarification: https://issues.dlang.org/show_bug.cgi?id=17448#c39
Feb 17
parent reply kinke <noone nowhere.com> writes:
On Wednesday, 17 February 2021 at 20:44:46 UTC, tsbockman wrote:
 On Wednesday, 17 February 2021 at 20:18:53 UTC, Paul Backus 
 wrote:
 On Wednesday, 17 February 2021 at 19:42:00 UTC, tsbockman 
 wrote:
 A copy constructor and opAssign can be used to update 
 pointers that are relative to &this:
     https://dlang.org/spec/struct.html#struct-copy-constructor
Unfortunately this is not enough, because the compiler is free to implicitly move struct instances in memory any time it wants to. See the bug report below for more details: https://issues.dlang.org/show_bug.cgi?id=17448 Until D gets move constructors, structs with interior pointers should be avoided.
That bug is about postblits, this(this), not copy constructors: this(ref typeof(this)). Copy constructors were added to the language specifically to fix those sort of problems.
Nope, Paul is right, the copy ctors don't solve anything in this regard. Simplest example I could come up with: https://run.dlang.io/is/TgxyU3
Feb 18
next sibling parent reply vitamin <vit vit.vit> writes:
On Thursday, 18 February 2021 at 08:29:48 UTC, kinke wrote:
 On Wednesday, 17 February 2021 at 20:44:46 UTC, tsbockman wrote:
 On Wednesday, 17 February 2021 at 20:18:53 UTC, Paul Backus 
 wrote:
 [...]
That bug is about postblits, this(this), not copy constructors: this(ref typeof(this)). Copy constructors were added to the language specifically to fix those sort of problems.
Nope, Paul is right, the copy ctors don't solve anything in this regard. Simplest example I could come up with: https://run.dlang.io/is/TgxyU3
opPostMove https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1014.md can solve this problem but it isn't implemented;
Feb 18
parent Paul Backus <snarwin gmail.com> writes:
On Thursday, 18 February 2021 at 11:00:50 UTC, vitamin wrote:
 opPostMove 
 https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1014.md can solve
this problem but it isn't implemented;
IIRC opPostMove has been abandoned for the same reason postblits were abandoned (issues with type-checking and const/immutable). Walter has a draft DIP that introduces move constructors [1], but it is currently on hold because of a new rule requiring that DIPs authored by a Language Maintainer (i.e., Walter or Atila) have a third-party "champion" to take them through the DIP process. So, if anyone wants to "adopt" this DIP, it would be a great service to the community. [1] https://github.com/dlang/DIPs/pull/182
Feb 18
prev sibling parent tsbockman <thomas.bockman gmail.com> writes:
On Thursday, 18 February 2021 at 08:29:48 UTC, kinke wrote:
 Nope, Paul is right, the copy ctors don't solve anything in 
 this regard. Simplest example I could come up with: 
 https://run.dlang.io/is/TgxyU3
I found that example very confusing, as it does not contain an explicit copy constructor, violate memory safety, or output incorrect results. However, I experimented with it and I think figured out what you're getting at? Copy constructors (and postblits) may not be called for moves. I guess that makes sense... /////////////////////////// import std.stdio : write, writeln; struct S { private const(S*) self, old = null; this(bool refSelf) inout pure trusted { if(refSelf) self = &this; } disable this(this) inout pure safe; this(scope ref inout(typeof(this)) that) inout pure trusted { self = &this; old = &that; } void print() const safe { write(&this); if(old is null) write(" (never copied)"); else write(" (last copied from ", old, " to ", self, ")"); writeln(" in sync: ", self is &this); } } S makeS() safe { S s = true; s.print(); return s; // NRVO } void takeS(S s) safe { s.print(); } void main() safe { S s = true; s.print(); takeS(s); takeS(makeS()); } /////////////////////////// This works on LDC, but fails on DMD with output: 7FFF765249A0 (never copied) in sync: true 7FFF765249C0 (last copied from 7FFF765249A0 to 7FFF765249D0) in sync: false 7FFF76524A00 (never copied) in sync: true 7FFF765249F0 (never copied) in sync: false (It's weird that DMD runs the copy constructor with a destination address that isn't even the real destination.) Anyway, thanks for helping me understand, everyone.
Feb 18