RFR: JDK-8270308: Amalloc aligns size but not return value (take 2) [v3]

[Thomas Stuefe]

On Wed, 21 Jul 2021 15:19:12 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi,
>> 
>> may I please have reviews for this fix. This fixes an issue with arena allocation alignment which can only happen on 32-bit. 
>> 
>> The underlying problem is that even though Arenas offer ways to allocate with different alignment (Amalloc and AmallocWords), allocation alignment is not guaranteed. This sequence will not work on 32-bit:
>> 
>> 
>> Arena ar;
>> p1 = ar.AmallocWords();  // return 32bit aligned address
>> p2 = ar.Amalloc();       // supposed to return 64bit aligned address but does not.
>> 
>> 
>> This patch is the bare minimum needed to fix the specific problem; I proposed a larger patch before which redid arena alignment handling but it was found too complex: https://github.com/openjdk/jdk/pull/4784 . 
>> 
>> This fix is limited to `Amalloc()` and aligns `_hwm` to be 64bit aligned before allocation. But since chunk boundaries are not guaranteed to be 64-bit aligned either, additional care must be taken to not overflow `_max`.  Since this adds instructions into a hot allocation path, I restricted this code to 32 bit - it is only needed there.
>> 
>> Remaining issues:
>> 
>> - `Amalloc...` align the allocation size in an attempt to ensure allocation alignment. This is not needed nor sufficient, we could just remove that code. I left it untouched to keep the patch minimal. I also left the `// ... for 32 bits this should align _hwm as well.` comment in `Amalloc()` though I think it is wrong.
>> - The chunk dimensions are not guaranteed to be 64-bit aligned:
>> 	1) Chunk bottom depends on Chunk start. We currently align the header size, but if the chunk starts at an unaligned address, this is not sufficient. It's not a real issue though as long as Chunks are C-heap allocated since malloc alignment is at least 64bit on our 32bit platforms. More of a beauty spot, since this is an implicit assumption which we don't really check.
>> 	2) Chunk top and hence Arena::_max are not guaranteed to be 64-bit aligned either. They depend on the input chunk length, which is not even aligned for the standard chunk sizes used in ChunkPool. And users can hand in any size they want. Fixing this would require more widespread changes to the ChunkPool logic though, so I left it as it is.
>> 	3) similarly, we cannot just align Arena::_max, because that is set in many places and we need to cover all of that; it ties in with Arena rollback as well.
>> 
>> Because of (2) and (3), I had to add the overflow check into `Amalloc()` - any other way to solve this would result in more widespread changes.
>> 	
>> -----
>> 
>> Tests:
>> 
>> - I tested the provided gtest on both 64-bit and 32-bit platforms (with and without the fix, without it shows the expected problem)
>> - GHA
>> - Tests are scheduled at SAP.
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   kbarret feedback

> I think this still isn't right.
>...
> The latest version doesn't seem any better than the previous. It could still return a misaligned value in the same circumstance, just more slowly because of the additional branch.

How? I must be blind.

- If size > 0: Amalloc aligns _hwm to 64-bit and returns a 64-bit aligned address -> ok
- if size == 0: Amalloc does not change the arena, but returns a potentially unaligned pointer - but that is fine too since that pointer cannot be used for storing anything. Certainly nothing needing 64-bit alignment.

What am I overlooking here?

> Under what circumstances can _hwm not be 64bit aligned? What needs to be done to prevent it?

_hwm is misaligned by the caller mixing Amalloc and AmallocWords on 32-bit.

AmallocWords advances _hwm by word size. Then, it is not 64-bit aligned anymore. A subsequent Amalloc() call needs to return a 64-bit aligned pointer, so it needs to align _hwm up. A subsequent AmallocWords() call, however, would be fine with _hwm as it is, so the alignment should really be done at Amalloc().

We cannot just align the allocation size to always be 64-bit, since on an Arena that uses repeated calls to AmallocWords() (e.g., HandleArea) this would create allocation padding and waste memory or even crash if the arena cannot handle padding.

> It looks to me that if Arena::grow were to align its size argument, then it all falls out, assuming std::max_align_t is also appropriately aligned. 

No, since this only ensures the chunk boundaries are properly aligned. The first call to AmallocWords - on a 32-bit platform - will misalign _hwm for potential follow-up calls to Amalloc().

In short, mixing allocation calls with different alignment requirements means the calls with higher alignment guarantees must align. Alternatively, you must ensure that for a given Arena, only one alignment is used - I proposed that alternative in my first patch. Make alignment property of the Arena and only allow allocations with that alignment.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4835