issue 14134 - Free of large array does not work

[rainers]

https://issues.dlang.org/show_bug.cgi?id=14134

"delete slice" now destroys and frees the full array

[schveiguy]

Should we set *p to null if it's not a GC array? How does one realize that delete didn't actually do anything?

[rainers]

Not changing p if it's not freed sounds reasonable. Would that be a change to the language specification? Unfortunately the documentation is pretty short for "delete".

[rainers]

I've moved clearing the passed array into the conditional.

[schveiguy]

I can live with this, but I would wonder if we should add a check to ensure the array pointer points at the start of the array block. This would be consistent with how GC.free works.

[rainers]

can live with this, but I would wonder if we should add a check to ensure the array pointer points at the start of the array block.

Yeah, this was also an option mentioned in the bug report comments. It seems only half-correct to check the start pointer, but not the length, though. Both indicate the caller might not actually "own" the array.

[rainers]

It seems only half-correct to check the start pointer, but not the length, though.

I just notice that we cannot always check the length, because we only have type info for structs with dtors.

[schveiguy]

Yeah, this was also an option mentioned in the bug report comments. It seems only half-correct to check the start pointer, but not the length, though. Both indicate the caller might not actually "own" the array.

But it's very normal to truncate with arr.length -= .... Slicing off the front is not as expected.

The point I was making is that we already have a requirement that GC.free will only work with a block pointer. However, at this point, I'm not too worried since delete is going away anyway, so I'm fine with what you have.

I just notice that we cannot always check the length, because we only have type info for structs with dtors.

delete should give you the static TypeInfo of the element, which is all you need to see if the overall length matches what's stored in the array block.

But in any case, we probably shouldn't spend too much time on this, see previous comment ;)

[schveiguy]

delete should give you the static TypeInfo of the element,

I take this back, I see it doesn't actually! (though it could) So yeah, no way to check that unfortunately.

[MartinNowak]

Both indicate the caller might not actually "own" the array.

Both GC.free and delete are unsafe operations exactly because they can delete memory they don't own. Requiring that delete is only called for a "complete" array doesn't make it any safer.

[MartinNowak]

Why did you add the appendable check?
I'm just wondering because I want to add thread local caches to the GC and need to sync any bits lazily as it's too expensive otherwise.

[schveiguy]

Don't you need to know if APPENDABLE is there in order to verify the array to be deleted starts at the right point?

Edit: Forget this comment, he didn't implement that. I agree you don't need APPENDABLE.

That being said, why would this check be improper?

[rainers]

Why did you add the appendable check?

Should we also free anything that was not allocated as an array by the runtime? Running dtors is dangerous in this case, though we could check flags only in case a type info is passed.

[MartinNowak]

Not sure, but I'd tend to check APPENDABLE too, because of this.

class Foo
{
    static struct Bar { ~this() {} }
    Bar[4] bars;
}

unittest
{
    auto foo = new Foo;
    auto bars = foo.bars[];
    delete bars;
}

[schveiguy]

Hm... that doesn't seem like the right correlation.

For instance:

auto foo = new Foo[2];
auto bars = foo[0].bars[];
delete bars;

I wonder if the type info to call destuctors shouldn't be read from the GC instead of relying on the static type. I didn't review that system that adds finalizer capability to structs. Is this what it is supposed to do?

[rainers]

It could be used, but it would not allow to manually emplace structs into a byte array and delete them later:

auto foo = new ubyte[2*Bar.sizeof];
auto bars = cast(Bar[])foo;
delete bars;

[schveiguy]

Well, in that case, you are on your own anyway :)

// delete bars;
bars = null; // let GC destroy... oops

[rainers]

Well, that would not call any destructors. I think that might be expected.

[schveiguy]

I don't know whether it's expected or not, but I think I might expect either possibility when I call delete.

What does make the most sense to me is to have delete be a "pre-emptive GC collection," so everything that is done via GC would be done via delete, except synchronously.

In any case, we are spending time debating a to-be-deprecated feature :) Nice to have these discussions though, for when we do actually have to create a replacement.

[MartinNowak]

debating a to-be-deprecated feature

It's probably TBD since 3 years, yet people keep using it.

[schveiguy]

@MartinNowak did you accidentally merge this?

[MartinNowak]

@MartinNowak did you accidentally merge this?

No, it fixes the issue for now. But I thought we might have some more ideas on the topic.

[schveiguy]

OK, I just was confused with the lack of auto-merge ;)

[WalterBright]

When writing pull requests that fix a bugzilla issue, write the title as:

Fix Issue NNNN ....

so that the bugzilla issue ill be automatically marked as fixed when it gets pulled.

[rainers]

When writing pull requests that fix a bugzilla issue, write the title as:
Fix Issue NNNN ....

I was not sure this is a fix, it was meant as a contribution towards a fix. Is calling delete going to be the proposed way to manually free an array from memory?