Issue 23556 - Array append ignores copy constructor
Summary: Array append ignores copy constructor
Status: NEW
Alias: None
Product: D
Classification: Unclassified
Component: druntime (show other issues)
Version: D2
Hardware: All All
: P3 normal
Assignee: No Owner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-12-14 19:02 UTC by Paul Backus
Modified: 2024-12-07 13:42 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Paul Backus 2022-12-14 19:02:26 UTC
As of DMD 2.101.0, the following program compiles and runs successfully to completion:

---
struct S
{
    this(ref S)
    {
        assert(0);
    }
}

void main()
{
    S[] a = [ S() ];
    auto p = a.ptr;
    // append until reallocation
    while (a.ptr == p)
        a ~= S(); // no assert
}
---

This program should assert at runtime when the array is reallocated, but does not, because the copy constructors of the array's elements are not called.

If the copy constructor is changed to a postblit (`this(this)`), the program asserts at runtime, as expected.

See also issue 20879, which affected .dup and .idup.
Comment 1 Steven Schveighoffer 2022-12-14 19:20:15 UTC
Just to give some clearer picture of how this causes issues:

```d
import std.typecons;
import std.sumtype;
import std.stdio;
import core.memory;

void fun(RefCounted!string foo) {
    SumType!( RefCounted!(string) )[] foo_storage;

    foo_storage ~= SumType!( RefCounted!(string) )(foo);
    foo_storage ~= SumType!( RefCounted!(string) )(foo); // reallocates, but no copy
    foo_storage = null;
}

void clobber()
{
    int[1000] x = 0x123456;
}

void main(){
    RefCounted!( string ) foo = RefCounted!( string )("blah");

    fun(foo);
    clobber();
    GC.collect(); // collect the items already in the GC
    writeln(foo);
}
```

What happens here is:

1. An array of one element is created with the sumtype.
2. The array cannot hold a second element, so it's reallocated. However, the copy constructor of SumType is *not* called. Therefore, `foo` has a ref count of 3, but there are afterwards 4 references (the single element array, the double-element array, and the original foo on the stack).
4. clobber makes sure the stack doesn't point at the arrays.
5. GC.collect cleans up 3 foo elements, reducing the count to 0 and deallocating.
6. The writeln is now writing garbage. on my system it just spewed random memory to the screen.
Comment 2 Teodor Dutu 2022-12-16 17:12:25 UTC
a ~= S() is not supposed to call either the copy constructor or the postblit because the newly appended element is supposed to be created in-place inside the array. This test ensures this behaviour (only the constructor is called, without the postblit): https://github.com/dlang/dmd/blob/d405b343e301e5c37a90e66131b3f4d588bed2f2/compiler/test/runnable/sdtor.d#L2244-L2245

The issue comes from copying the array during reallocation. The postblit is correctly called while the cpctor is not. The following code prints "loop" 15 times before printing "pblit" 15 times when the array is reallocated:

---
struct S
{
    this(this)
    {
        writeln("pblit");
    }
}

void main()
{
    S[] a = [ S() ];
    auto p = a.ptr;
    // append until reallocation
    while (a.ptr == p)
    {
        writeln("loop");
        a ~= S(); // no assert
    }
}
---
Comment 3 Steven Schveighoffer 2022-12-16 19:47:17 UTC
Yes, the problem is the copying of the existing data.

I can think of a few ways to fix this, but they aren't pretty. We either have to add more special members to TypeInfo, or change the hook so it can explain to the runtime how to call the copy constructor.
Comment 4 dlangBugzillaToGithub 2024-12-07 13:42:18 UTC
THIS ISSUE HAS BEEN MOVED TO GITHUB

https://github.com/dlang/dmd/issues/17454

DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB