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

[Jorn Vernee]

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

Marked as reviewed by jvernee (Committer).

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

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