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

Jorn Vernee jvernee at openjdk.java.net
Thu May 14 11:38:37 UTC 2020

On Wed, 13 May 2020 17:54:25 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> 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.

A nice chunk of code getting vaporized ��. As mentioned offline, the only problem I see is with structs where the
fields cross eightbyte boundaries (for instance when using packed structs).

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java line 528:

> 527:                 }
> 528:             }
> 529:         } else if (l.isPadding()) {

This loop is duplicated in the other method above. Might want to move to helper method.


Marked as reviewed by jvernee (Committer).

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

More information about the panama-dev mailing list