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

JDK-8242127: reorganize ABI-dependent layout constants #97

Closed

Conversation

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented Apr 3, 2020

There are two main problems with the way in which ABI-dependent constants are dealt with:

  • the SystemABI.Type enum abstraction is rarely used - code just prefers to go with layouts constants in MemoryLayouts
  • the current enum doesn't scale to model platform-dependent ABI types, so it just has to work with the set of types that are supported in all platforms

This patch rearranges things a bit. First, it removes the SystemABI.Type abstraction, and associated SystemABI::layoutFor method. Then, it moves all ABI layout constants from MemoryLayouts to SystemABI. Instead of using the enum value as a classification value for the ABI, each ABI constant holder defines its own classification constants. So there is a SystemABI.Win64.ArgumentClass, and a SystemABI.SysV.ArgumentClass and so on and so forth. A notable fact is that the set of classification values that are required by the ABI is much, much smaller than the set of "interesting" C types.

I then adjusted the implementation code not to no longer depend on the SystemABI.Type enum; while doing so I realized (suggestion from Jorn) that the public ArgumentClass enums would actually, both in Windows and AArch be enough to also implement the internal classification system. On SysV, since the classification is quite convoluted, there is a number of 'synthetic' classification classes that are only introduced when classifying struct (e.g. SSE_UP) which would be bad to expose outside - but we need these classes to keep the classification algorithm going. So, I moved ArgumentClassImpl under the sysv folder and kept the code as it was.

The resulting code is, I think more direct, and more scalable. We can keep adding layouts to SystemABI - both platform-independent and platform-dependent ones - they are just different sets of constants.


Progress

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

Issue

  • JDK-8242127: reorganize ABI-dependent layout constants

Reviewers

  • Jorn Vernee (jvernee - Committer)
  • Athijegannathan Sundararajan (sundar - Committer) ⚠️ Review applies to 57ea2dc

Download

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

mcimadamore added 2 commits Apr 3, 2020
* Use ABI specific enums to convey classification info
* Move layout constants to SystemABI
* remove SystemABI.layoutFor
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 3, 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 Apr 3, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 3, 2020

Webrevs

Copy link
Member

@JornVernee JornVernee left a comment

Looks good. I left a few comments inline.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 6, 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:

JDK-8242127: reorganize ABI-dependent layout constants

Reviewed-by: jvernee, sundar
  • 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.

➡️ To integrate this PR with the above commit message, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Apr 6, 2020
@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented Apr 6, 2020

/integrate

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

@openjdk openjdk bot commented Apr 6, 2020

@mcimadamore
Pushed as commit f7e7b9d.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 6, 2020

Mailing list message from Maurizio Cimadamore on panama-dev:

Changeset: f7e7b9d
Author: Maurizio Cimadamore <mcimadamore at openjdk.org>
Date: 2020-04-06 13:51:07 +0000
URL: https://git.openjdk.java.net/panama-foreign/commit/f7e7b9d6

JDK-8242127: reorganize ABI-dependent layout constants

Reviewed-by: jvernee, sundar

! src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryLayouts.java
! src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SystemABI.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/Utils.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/AArch64ABI.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java
- src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/ArgumentClassImpl.java
+ src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/sysv/ArgumentClassImpl.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVx64ABI.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/windows/CallArranger.java
! src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/x64/windows/Windowsx64ABI.java
! test/jdk/java/foreign/CallGeneratorHelper.java
! test/jdk/java/foreign/Cstring.java
! test/jdk/java/foreign/NativeTestHelper.java
! test/jdk/java/foreign/StdLibTest.java
! test/jdk/java/foreign/TestCircularInit1.java
! test/jdk/java/foreign/TestCircularInit2.java
! test/jdk/java/foreign/TestUpcall.java
! test/jdk/java/foreign/TestVarArgs.java
! test/jdk/java/foreign/callarranger/TestAarch64CallArranger.java
! test/jdk/java/foreign/callarranger/TestSysVCallArranger.java
! test/jdk/java/foreign/callarranger/TestWindowsCallArranger.java
! test/micro/org/openjdk/bench/jdk/incubator/foreign/CallOverhead.java
! test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java

@mcimadamore mcimadamore deleted the mcimadamore:abi-const-cleanup branch Apr 7, 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

3 participants
You can’t perform that action at this time.