[foreign-memaccess+abi] RFR: 8303017: Passing by-value structs whose size is not power of 2 doesn't work on all platforms

[Maurizio Cimadamore]

On Fri, 24 Feb 2023 21:08:50 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

> Fix passing of by-value structs whose size is not a power of 2.
> 
> The issue is that currently we try to do loads using a ValueLayout that is the size of the nearest power of two that can fit the struct (or part of it, if it is passed in multiple registers). For instance, for a struct of size 6, we try to load its value using `ValueLayout.OfLong`, which is size 8, and thus produce an out of bounds error. A similar issue applies to writes.
> 
> For the solution I've implemented in this patch, I've attached an explicit byte width to BufferLoads/Stores, which indicates the size of the value we want to load/store. The type that is produced by the binding is still the same. For example, loading a struct of size 6 is implemented as an `int` load and a `short` load, which are then combined into a `long`, instead of attempting to do a single `long` load (and failing). This allows us to avoid doing an out of bounds access.
> 
> I've added a new test that tests a bunch of structs with varying byte sizes being passed in registers and on the stack. Using a nested `char[]` to precisely tweak the byte size of each struct.

src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 591:

> 589:                 // non-exact match, need to do chunked load
> 590:                 long longValue = ((Number) value).longValue();
> 591:                 int remaining = byteWidth();

There is a strong assumption here that byteWidth() is always < 8. Perhaps add a comment.

src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 596:

> 594:                     int chunkSize = Integer.highestOneBit(remaining); // next power of 2, in bytes
> 595:                     long writeOffset = offset() + SharedUtils.pickChunkOffset(chunkOffset, byteWidth(), chunkSize);
> 596:                     int shiftAmount = chunkOffset * Byte.SIZE;

Is this correct? E.g. assume we have to write 3 shorts { 13, 14, 15 }, and we do that in two chunks. If we are big endian, I think we first write { 14, 15 } then { 13 }. When we write { 14, 15 }, Isn't the offset of the mask 2 bytes (e.g. 16 bits) ? In other words shouldn't `chunkOffset` here also be decorated with `pinkChunkOffset` to take endianness into account? (it seems to me that the shift amount should be equal to `writeOffset - offset()`).

src/java.base/share/classes/jdk/internal/foreign/abi/Binding.java line 662:

> 660:                             throw new IllegalStateException("Unexpected chunk size for chunked write: " + chunkSize);
> 661:                     };
> 662:                     result |= readChunk << (chunkOffset * Byte.SIZE);

same here - is the naked reference to `chunkOffset` ok (w.r.t. endianness) ?

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

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