Re: Struct delegate access corruption

2021-02-18 Thread tsbockman via Digitalmars-d-learn

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 = 
}
@disable this(this) inout pure @safe;
this(scope ref inout(typeof(this)) that) inout pure @trusted {
self = 
old = 
}

void print() const @safe {
write();
if(old is null)
write(" (never copied)");
else
write(" (last copied from ", old, " to ", self, ")");
writeln(" in sync: ", self is );
}
}

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.


Re: Struct delegate access corruption

2021-02-18 Thread Paul Backus via Digitalmars-d-learn

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


Re: Struct delegate access corruption

2021-02-18 Thread vitamin via Digitalmars-d-learn

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;




Re: Struct delegate access corruption

2021-02-18 Thread kinke via Digitalmars-d-learn

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 :

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


Re: Struct delegate access corruption

2021-02-17 Thread tsbockman via Digitalmars-d-learn

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 :

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.


However, since issue #17448 is still open I have posted a 
question to the issue report asking for clarification:

https://issues.dlang.org/show_bug.cgi?id=17448#c39


Re: Struct delegate access corruption

2021-02-17 Thread Paul Backus via Digitalmars-d-learn

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 = 
}
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 :

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.


Re: Struct delegate access corruption

2021-02-17 Thread tsbockman via Digitalmars-d-learn

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 = 
}
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 :

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



Re: Struct delegate access corruption

2021-02-17 Thread H. S. Teoh via Digitalmars-d-learn
On Wed, Feb 17, 2021 at 03:38:00PM +, 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 = 
}
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


Re: Struct delegate access corruption

2021-02-17 Thread Steven Schveighoffer via Digitalmars-d-learn

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, );
T b = a.d();//the delegate

While in the most used delegate :

writefln!"test2 %s %s"(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


Struct delegate access corruption

2021-02-17 Thread z via Digitalmars-d-learn
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, );
T b = a.d();//the delegate

While in the most used delegate :

writefln!"test2 %s %s"(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