Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8244938: Crash in foreign ABI CallArranger class when a test native function returns a nested struct #162

Closed

Conversation

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 13, 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.


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

  • JDK-8244938: Crash in foreign ABI CallArranger class when a test native function returns a nested struct

Reviewers

  • Jorn Vernee (jvernee - Committer)

Download

$ git fetch https://git.openjdk.java.net/panama-foreign pull/162/head:pull/162
$ git checkout pull/162

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 13, 2020

👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into foreign-abi will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label May 13, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 13, 2020

Webrevs

Copy link
Member

@JornVernee JornVernee left a comment

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

for (MemoryLayout m : group.memberLayouts()) {
groupByEightBytes(m, offset, groups);
if (group.isStruct()) {
offset += m.byteSize();
}
}
Comment on lines +523 to +528

This comment has been minimized.

@JornVernee

JornVernee May 14, 2020
Member

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

@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2020

@mcimadamore This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

8244938: Crash in foreign ABI CallArranger class when a test native function returns a nested struct

Reviewed-by: jvernee
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 2 commits pushed to the foreign-abi branch:

  • 4d1d5bf: Automatic merge of foreign-memaccess into foreign-abi
  • 667f7f0: Simplify MemoryScope implementation to use StampedLock

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge foreign-abi into your branch, and then specify the current head hash when integrating, like this: /integrate 4d1d5bfd2581756b401551012d477b262a15c66c.

➡️ To integrate this PR with the above commit message to the foreign-abi branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 14, 2020
@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented May 14, 2020

/integrate

@openjdk openjdk bot closed this May 14, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels May 14, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 14, 2020

@mcimadamore The following commits have been pushed to foreign-abi since your change was applied:

  • 4d1d5bf: Automatic merge of foreign-memaccess into foreign-abi
  • 667f7f0: Simplify MemoryScope implementation to use StampedLock

Your commit was automatically rebased without conflicts.

Pushed as commit 32d1f6c.

@mcimadamore mcimadamore deleted the mcimadamore:sysvNestedStructFix branch May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants