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

[Kim Barrett]

On Sat, 24 Jul 2021 18:04:29 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:
> 
>   make sure Chunk payload boundaries are 64-bit aligned

This looks much better.  But I think you missed a place.

If AmallocWords is called with a non-64bit-aligned value (it could be only 32bit aligned on a 32bit platform), and it calls grow(), grow will call the chunk allocator with that length, which fails the precondition because it's not BytesPerLong aligned. I think grow() needs to call ARENA_ALIGN on the length on 32bit platforms.

That also suggests an additional test.

I like the new comment for Chunk::operator new.

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

Changes requested by kbarrett (Reviewer).

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