[foreign-memaccess+abi] RFR: 8300294: Add tests for by-value unions and structs with nested fixed-length arrays

[Jorn Vernee]

On Thu, 9 Feb 2023 10:14:30 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> I've added some tests for by-value unions, structs/unions nested into other structs, and aggregates with nested inline arrays.
>> 
>> These test cases are taken from a fuzzer I wrote a while ago. So, they are a bit random in terms of the generated layouts, but should nonetheless  serve as a basic test for the cases that we miss with TestDowncall/TestUpcall today. This mostly helps give some coverage of the linker's classification code for these cases.
>> 
>> I've also pulled in some test value generation code, which can generate random test values when given a MemoryLayout, allocator and random generator. (if it seems useful, I can re-write the existing tests to use this generation code as well, but I'd like to save that for a followup PR).
>> 
>> Unfortunately, the fallback linker doesn't seem to be able to reliably handle by-value unions. It's a know issue with libffi. I've filed: https://bugs.openjdk.org/browse/JDK-8301800 for now. I've changed the implementation to throw an IAE, which was already specified for `Linker::downcallHandle/upcallStub`: 
>> 
>> https://github.com/openjdk/panama-foreign/blob/f61f3a31af4976d0e64d3bfa72cda95b501e2a7d/src/java.base/share/classes/java/lang/foreign/Linker.java#L232-L235
>> 
>> There was also a bug in the classification of HFAs on AArch64 which I've fixed. The issue was that structs with float/double fields that are nested into arrays or other structs/unions should also be counted as HFAs. But, the correct classification logic only accounted for the 'flat' case, where a struct only contains ValueLayouts.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java line 269:
> 
>> 267:             int regType = forHFA ? StorageType.VECTOR : StorageType.INTEGER;
>> 268:             int numChunks = (int)Utils.alignUp(layout.byteSize(), MAX_COPY_SIZE) / MAX_COPY_SIZE;
>> 269:             List<MemoryLayout> scalarLayouts = forHFA ? TypeClass.scalarLayouts(layout) : null;
> 
> Perhaps, instead of two conditionals, it would be better to use a simple `if` ? I'm a bit worried about one path being `null` which then works in the subsequent conditional because the leading condition is the same. Seems a bit brittle.

That's also a possibility, I can put the initialization of `regType` into the `if` as well. I'll do that.

> src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/TypeClass.java line 70:
> 
>> 68:     }
>> 69: 
>> 70:     private static void scalarLayoutsR(List<MemoryLayout> out, GroupLayout gl) {
> 
> where does the `R` in the name comes from?

R stands for 'recursive'. I'll type it out. (or, maybe another name like `scalarLayoutsInternal` is better. That seems to be more common in JDK code at least)

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

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