[foreign-memaccess+abi] RFR: 8284300: Explicitly reject layouts that don't have the correct alignment during linking

[Jorn Vernee]

On Tue, 20 Sep 2022 17:44:41 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> I realized that rejecting layouts that don't have natural alignment also prevents us from supporting packed structs in the future. I tested out the packed struct support and it works on Windows pretty much out of the box. It doesn't work on SysV, some changes to CallArranger are needed. So, there are some options:
>> 1. reject layouts that don't have natural alignment on all platforms (Windows won't be able to use packed structs)
>> 2. reject layouts that don't have natural alignment on platforms besides Windows (yay Windows users)
>> 3. align the status quos: Windows and AArch64 re-compute the alignment. It's not used on Windows, so it can be dropped there. On AArch64 we could "trust" the alignment that is attached to the layout given by the user, same as SysV, but it would fail for packed structs. With the intention of fixing the failure in the future.
>> 
>> I'm leaning most towards 1 at the moment, and that's what this patch implements. We can change that approach when we add reliable support for packed structs across platforms.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 279:
> 
>> 277:                 long copy = Math.min(layout.byteSize() - offset, COPY_CHUNK_SIZE);
>> 278:                 VMStorage storage = storageCalculator.stackAlloc(copy, align);
>> 279:                 align = 1; // only first copy needs aligning
> 
> so, this was an issue in the old code? (seems like in old code we always align to STACK_SLOT_SIZE)

Sorry, forgot to explain this.

In the old code we always passed STACK_SLOT_SIZE. This works out in the current implementation since we only allow values to be copied to the stack at stack slot aligned offset. Though, technically, we might need a more precise alignment in the case of Mac/AArch64. For instance, if a struct has 4 byte alignment, we might need to spill it at a 4-byte aligned stack offset.

I noticed this when looking through the code and decided to fix this. But, I'm think now that it kinda doesn't belong in this patch. I can remove it. All we need is the change to drop `SharedUtils.alignment` for `layout.byteAlignment` above.

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

PR: https://git.openjdk.org/panama-foreign/pull/728