Fix issue 18996 - ProtoGC should support removing roots and ranges that were not originally added

[schveiguy]

The original behavior was to assert(false), but this is inconsistent with the standard GC, which ignores removal of ranges it didn't already know about.

The code in question, Array!string, whenever it would realloc, it would removeRange on the previous array, and addRange on the new one. Since it starts out with a null array, removeRange(null) was called.

Arguably, this is not behavior we want (Array could easily be changed to not try and removeRange(null)), but the ProtoGC shouldn't behave differently from the actual GC while it is in charge of things.

Added a simple test to make sure this doesn't happen again.

[dlang-bot]

Thanks for your pull request, @schveiguy!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	18996	regression	Inserting a type containing indirections into an std.container Array causes SIGILL(4). Illegal Instruction.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + druntime#2220"

[schveiguy]

ping @MartinNowak this should go into 2.081.0, it's a regression.

[wilzbach]

@schveiguy as long as it's targeted on stable (which it is) and merged before the end of this month, it will be part of the final 2.081

[rainers]

LGTM. The documentation in core.memory also states that nothing happens for null or unknown pointers.

The implementation is still slightly different, because the GC does not remember adding a root or range twice, so a single removeRoot/Range can lead to subtle bugs. ProtoGC remembers identical roots/ranges multiple times, so might keep them alive longer.

[schveiguy]

ProtoGC remembers identical roots/ranges multiple times, so might keep them alive longer.

That's a simple fix as well, just don't return :)

[schveiguy]

I think jenkins wasn't coming back with a test, apparently you have to close and reopen.

[wilzbach]

@schveiguy ci.dlang.io is currently down. It will never reply.

[schveiguy]

Hm... so I guess I'll use the auto-tester to merge. Thanks for the update.

[schveiguy]

Auto-merge toggled on

[wilzbach]

Hm... so I guess I'll use the auto-tester to merge. Thanks for the update.

That won't help you, because it can't merge when there are required CI checks that haven't passed yet.
I do have admin access and can still merge this pull request (exactly for these cases).
I will do this for this PR now.
I hope that ci.dlang.io gets back online soonish, but I haven't heard back from Martin.

[schveiguy]

Ah thanks, I was confusing this case with the one where the bot won't merge when non-required CIs haven't reported.

[rainers]

That's a simple fix as well, just don't return :)

Yes, but I think the returning API is much saner. If you have two independent sources that happen to add the same root/range, it is very likely that the root/range is still in use after one source has released it.

[schveiguy]

If you have two independent sources that happen to add the same root/range, it is very likely that the root/range is still in use after one source has released it.

Agreed. So maybe it's the conservative GC that needs updating.