[foreign-abi] [Rev 01] RFR: 8244938: Crash in foreign ABI CallArranger class when a test native function returns a nested struct
jvernee at openjdk.java.net
Thu May 14 13:43:34 UTC 2020
On Thu, 14 May 2020 13:35:02 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(
>> 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(
>> 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
Marked as reviewed by jvernee (Committer).
More information about the panama-dev