[foreign-abi] [Rev 01] RFR: 8244938: Crash in foreign ABI CallArranger class when a test native function returns a nested struct

[Maurizio Cimadamore]

> This is a nasty issue with the SysV struct classification algorithm (which is rather complex).
> 
> The algortigm as specified, is designed to work on 8-byte chunks at a time (the SysV spec calls them `eighbytes`). This
> means that when there are nested structs, some structs might need to be broken apart to implement the algorithm
> correctly. Consider this case:  MemoryLayout POINT = MemoryLayout.ofStruct(
>                 C_INT.withName("x"),
>                 MemoryLayout.ofStruct(
>                         C_INT.withName("y"),
>                         C_INT.withName("z")
>                 )
>         );
> 
> Here we have an int field (`x`) and then two nested int fields (namely, `y` and `z`).
> 
> The currently implemented logic, sees the first field, and classifies it as `INTEGER`. It then calls the classification
> recursively on the second field, which is a struct. Since the struct fits in 8 bytes, the recursive classification
> yields a single class, namely INTEGER. The outer classification logic then attempts two merge the two INTEGER classes
> (one from the first field, the other from the struct), and obtain a *single* INTEGER class as a result. This is a wrong
> result, as 12 bytes cannot obviously fit in a single eightbyte.  To address this issue I first considered flattening
> structs, but then I quickly gave up, since it was pretty messy to make sure that flattening worked correctly with
> respect to unions (e.g. structs inside unions).  I then settled on a simpler scheme: since the classification logic is
> meant to work on one eightbyte at a time, I just wrote a routine that parses the incoming `GroupLayout` and breaks it
> apart into an array of `ArgumentClassImpl`, where the size of the array is the number of eightbytes into which the
> group is to be classified.  We recursively scan the layout, trying to find all the fields, and keeping track of their
> offsets. Eventually, when we come to leaves layouts (values) we add their corresponding ArgumentClassImpl to the array
> slot that corresponds to the eightbyte associated with the offset being considered.  Once this processing is done,
> classifying the struct is a breeze, as what's left to do is simply to *merge* all the classes in a single eightbyte
> slot (which can be done with a simple reduce step).  Note: for this logic to work, we have to assume that all value
> layouts in the group are not bigger than 8 bytes. In practice this is not a big issue, since bigger value layouts were
> not supported anyway. I also believe it won't be an issue moving forward, since we can simply make sure that e.g. the
> SysV type `__int128` is modelled with this layout:  MemoryLayout.ofStruct(C_INT, C_INT)  Or that `long double` is
> handle like this:  MemoryLayout.ofStruct(
>     C_LONG.withAttribute(ArgumentClass.X87),
>     C_LONG.withAttribute(ArgumentClass.X87_UP)
> 
> And so forth for vector types. In other words, rather than making the classification logic more complex, we can simply
> define the ABI layout constants accordingly, so that they are already broken up into 8-byte chunks.

Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:

  Add support (and tests) for structs containing unaligned fields

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

Changes:
  - all: https://git.openjdk.java.net/panama-foreign/pull/162/files
  - new: https://git.openjdk.java.net/panama-foreign/pull/162/files/729be0f4..8f88cf80

Webrevs:
 - full: https://webrevs.openjdk.java.net/panama-foreign/162/webrev.01
 - incr: https://webrevs.openjdk.java.net/panama-foreign/162/webrev.00-01

  Stats: 64 lines in 2 files changed: 63 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/panama-foreign/pull/162.diff
  Fetch: git fetch https://git.openjdk.java.net/panama-foreign pull/162/head:pull/162

PR: https://git.openjdk.java.net/panama-foreign/pull/162