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

Maurizio Cimadamore mcimadamore at openjdk.java.net
Wed May 13 17:59:57 UTC 2020


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.

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

Commit messages:
 - Partially rewritten logic for SysV arg classification to accommodate nested structs.

Changes: https://git.openjdk.java.net/panama-foreign/pull/162/files
 Webrev: https://webrevs.openjdk.java.net/panama-foreign/162/webrev.00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8244938
  Stats: 234 lines in 2 files changed: 91 ins; 113 del; 30 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


More information about the panama-dev mailing list