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 2021
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 2021
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 2021
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 2021
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 2021
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 2021
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 2021
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 2021
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 2021
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 2021