Fixed issue 14126 -- GC seemingly corrupting memory

[schveiguy]

The constant LARGEPAD was used instead of LARGEPREFIX when offsetting array elements in large arrays. LARGEPAD is used to determine total array size, not just the extra space at the beginning, and this includes 1 sentinel byte for avoiding cross-block issues. See https://issues.dlang.org/show_bug.cgi?id=14126 for a better explanation.

[schveiguy]

There is still a problem somewhere.

[schveiguy]

OK, it's fine, but there is another issue I have to ask about.

[dnadlinger]

Urgh, this is tricky. After spending some time building a mental model of all the different constants, I'm fairly confident that your change is correct. However, it would be nice if the meaning of the constants was documented better. As you probably have a more thorough understanding of the code than I do, could you maybe see about adding some explanations to the enum at the top?

[dnadlinger]

Auto-merge toggled on

[schveiguy]

could you maybe see about adding some explanations to the enum at the top?

Is this not good enough? https://github.com/schveiguy/druntime/blob/issue14126/src/rt/lifetime.d#L235

Although I probably could just add more to the enum docs.

[schveiguy]

OK, it's fine, but there is another issue I have to ask about.

While making this fix, I found this:

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

Any ideas? I know the GC just went through a huge overhaul, so I'm not sure where to look. From what I can tell, the code doesn't look wrong.

[dnadlinger]

I found it a bit non-obvious that small/medium refer to the two different length classes until looking at their values. Also, where does the /2 come from in "a block with 512 to pagesize/2 bytes has a 16-bit length"?

As for the other issue, I don't really have any ideas without having a closer look myself, sorry.

[rainers]

While making this fix

Thanks. I can't believe this slipped through the review ;-)

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

Any ideas? I know the GC just went through a huge overhaul, so I'm not sure where to look. From what I can tell, the code doesn't look wrong.

As mentioned in the bug report, this works as expected and documented. If you don't pass the base pointer of an allocation, you don't qualify as the "owner" of the block.

[schveiguy]

I found it a bit non-obvious that small/medium refer to the two different length classes until looking at their values.

ok.

Also, where does the /2 come from in "a block with 512 to pagesize/2 bytes has a 16-bit length"?

So each block is a power of 2. The largest power of 2 that can be represented by 1 byte length is 256 bytes (only because 255 bytes are available). After that (512 bytes minimum), you need a 2-byte length. The pagesize/2 is the largest block size, because the next size is pagesize. At that size, we switch to the other method of tracking length where it's stored at the front (and uses 4 or 8 bytes to store the length).

[dnadlinger]

Ah yes, I just forgot about the power of two requirement. Thanks.

[rainers]

So each block is a power of 2....

This assumes details of the GC implementation, especially that allocations above 2048 use full pages and are extendable. It get's slightly incorrect with the current GC if you enable the sentinel code (no more power of 2).

[schveiguy]

especially that allocations above 2048 use full pages and are extendable

The whole file does that. And I didn't add that concept to it, been there since D1 days.

If there was a way to parameterize the GC so we could have the GC define and tell us all its details, that would be helpful. But at this point, I don't think it's worth the effort.