Fix Appender extend

[monarchdodra]

Fixes the usage of "GC.extend" in Appender.

Long story short: Don't extend allocations you don't own: Even if it succeeds:

• It doesn't mean you own the extended space (let alone the data "up to" the start of the extension).
• You don't know the underlying memory layout (eg BlkAttr.APPENDABLE).

Fixes 12428, as well as the snippet submitted in 9092.

This edit changes the indentation, so I suggest viewing the diff without whites:
https://github.com/D-Programming-Language/phobos/pull/2076/files?w=1

[schveiguy]

I think the issue is more that you cannot assume the original block starts at the given pointer. I would actually rename the flag "canExtend", because block ownership is not necessarily the condition we care about. Some future version may be able to figure out how to extend properly with a passed-in block.

[monarchdodra]

True. I'll switch it.

[schveiguy]

Other than the inline comments, the code looks good.

[monarchdodra]

Other than the inline comments

Issues mostly addressed? Covering the bug is kind of difficult, while still keeping the test relevant.

[schveiguy]

Issues mostly addressed?

Yes, looks much better.

Covering the bug is kind of difficult, while still keeping the test relevant.

I would like to see a unit test that reliably fails on all platforms when the fix is not in place, and passes when it is not. I noticed you removed the very large memory test, which I agree should have been removed, but what about a more specific test like this:

1. Find the corner case, replicate it. For example, your 2048+1 test, then append enough to invoke "extend."
2. Use the GC to prove that the capacity of the appender extends beyond the block in which it is contained without the fix. I think GC.query on the data pointer should do it.

This does not rely on seg fault, yet proves it is doing something incorrect.

One possible dilemma with such a test is that you cannot be sure the original block would have been extended. You could do some GC work to make it so, basically allocate 2 pages, then release the upper page back to the GC. You may want to mark something like that as being very GC specific, but this bug is very GC specific anyway.

Anyway, without this kind of test, I think the change is still good to go. But if you feel like adding it, that would be great.

[monarchdodra]

I would like to see a unit test that reliably fails on all platforms when the fix is not in place

Even without segfaults, it is possible to observe certain issues, such as clobbering, and I covered that: The second unittest is a "barebone" version of what happened in 12428.

But if you feel like adding it, that would be great.

Another issue, is that Appender could (if it tracked that information), make an attempt to call reserve we initialized from a dynamic array with APPENDABLE info. There is an open request for size_t extend(T[] arr, size_t howMuch), which Appender would use if it could.

At this point, the unittest wouldn't be checking the behavior of Appender anymore, but rather, covering implementation specific details.

Furthermore, making assumptions on the GC will behave is very dangerous. For example, in my 2048 + 1 test, I was originally having problems replicating the issue, because I the "reference" allocated after the array: This made it occupy the space that would have been otherwise used to extend the array, and my unittest failed to replicate the issue. So I'm worried that even if the unittest is relevant now, it will would just bitrot real quick.

FYI: "I noticed you removed the very large memory test". Honestly, I had to go that high to reproduce a segfault, when initialy (in the original bug report), 61_000 + 5000 was enough. These things just aren't stable enough to be reliable. Example of bitrot I guess.

---

So it's not that I don't feel like adding a new test, it's just that I feel all the relevant are already there, and anything more would not be correct/relevant.

[schveiguy]

So it's not that I don't feel like adding a new test, it's just that I feel all the relevant are already there, and anything more would not be correct/relevant.

OK, fair enough.

[monarchdodra]

So, good to go?

[schveiguy]

Good by me, anyone else? Again, I'm not sure if it's good procedure to simply merge something that only I have looked at.

[Orvid]

Looks good to me.

[schveiguy]

Auto-merge toggled on

[monarchdodra]

Thanks for the review!