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-8244270: reorganize ABI-dependent layout constants (second attempt) #141

Closed

Conversation

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 1, 2020

This is another attempt to reorganize ABI-dependent layout constants. The constant reorg is identical to the one proposed in:

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

But this time I've also integrated the changes into the jextract branch (as illustrated in this PR).

The main realization which led to this work was that we don't need the ABI type enum: we already have the jextract primitive type enum. So, we could just define constants for the common ABI types which are useful for ABI classification in SystemABI - and then we could start from here in jextract, and augment these layouts by attaching an extra attribute with jextract's Primitve/Kind enum.

I've simplified the API a bit by moving the layout inside the enum itself (so that the layout can be accessed directly from the enum constant) - and also simplified the primitive type factory.

Everything worked out as expected - note how much duplicated layout creation we managed to get rid of.

One problem I had was that the loader we use inside jextract test was not delegating correctly (was using null as parent loader) so I had to fix that.

I'm not 100% sure that this jextract/type extra annotation makes sense - at the end of the day it is now only used inside tests (as it makes layout comparison easier - since layouts might have additional attributes such as name and such).

So I'm open to suggestion on whether we want to keep it or not - I believe that tests which look for specific layouts should instead probably check for specific types instead. Anyway, we can also start from this cleanup, and take care of these issues later; the important thing is that, with this rewrite we are finally able to cleanup the ABI constants the way we wanted.


Progress

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

Issue

  • JDK-8244270: reorganize ABI-dependent layout constants (second attempt)

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 1, 2020

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

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

@mlbridge mlbridge bot commented May 1, 2020

@slowhog
Copy link
Collaborator

@slowhog slowhog commented May 2, 2020

Overall I think this is a nice move, we want constants in SystemABI with proper MemoryLayout as we needed for access ABI specific type.

I don't like the Utils.pick where we need to pass in all ABI candidates, we have picked an implementation class at that point, we should be able to get those just from the implementation class. I would think instead of get rid of the enum, we may actually hide the types internally and have each ABI class provide layout. However, this can be clean up later, the important thing is that the constants are in place.

The other thing I don't think we should be doing is to make CLASS_ATTRIBUTE like "abi/sysv/class" part of the layout.

SystemABI constants should be a delegation to the implementation class, and strip the abi-class attribute. Trusting this attribute from external is dangerous, a tool can compose a bad layout to do some harm?

Bottom line, type is reasonable to expose to public, ABI implementation detail such as classification is not.

@slowhog
Copy link
Collaborator

@slowhog slowhog commented May 2, 2020

The other thing I don't think we should be doing is to make CLASS_ATTRIBUTE like "abi/sysv/class" part of the layout.

SystemABI constants should be a delegation to the implementation class, and strip the abi-class attribute. Trusting this attribute from external is dangerous, a tool can compose a bad layout to do some harm?

Bottom line, type is reasonable to expose to public, ABI implementation detail such as classification is not.

Actually expose type or classification have the same effect, as classification is derived from type, the attribute value is an instance of a hidden class type so it's somewhat protected.

@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented May 5, 2020

Bottom line, type is reasonable to expose to public, ABI implementation detail such as classification is not.

There's a tradeoff here. The ABI obviously needs more info than those attached to a layout to do the classification; otherwise the layout of a float and that of an int would be the same on most platform. One might argue - but why not just using the carrier types attached to the MethodType (also passed to the ABI) to do the classification? Because that's brittle - it works for primitives, it doesn't work for structs passed by value, which, on certain ABI (e.g. SysV) require a fair amount of recursive classification.

So the choices are between augmenting layouts (with attributes) and coming up with a new abstraction which is essentially a layout + some classification info (and maybe a Java carrier). In the name of minimality of the API surface we opted for the first option. Every time we considered moving away from layouts, the "bang for bucks" factor seemed relatively low.

@openjdk
Copy link

@openjdk openjdk bot commented May 5, 2020

@mcimadamore this pull request can not be integrated into foreign-jextract due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout jextract+abireorg
git fetch https://git.openjdk.java.net/panama-foreign foreign-jextract
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge foreign-jextract"
git push
@openjdk openjdk bot added merge-conflict and removed rfr labels May 5, 2020
@mcimadamore mcimadamore force-pushed the mcimadamore:jextract+abireorg branch from 0528829 to 2806ba5 May 5, 2020
@openjdk openjdk bot added rfr and removed merge-conflict labels May 5, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 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-8244270: reorganize ABI-dependent layout constants (second attempt)

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.

There are currently no new commits on the foreign-jextract branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate 6f69981b60125420f99cee4fc8b8d91754602a16.

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

@openjdk openjdk bot added the ready label May 6, 2020
@mcimadamore mcimadamore force-pushed the mcimadamore:jextract+abireorg branch from f2e297e to bbdfae0 May 6, 2020
mcimadamore and others added 2 commits May 6, 2020
…/impl/LayoutUtils.java

Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>
…t/tool/OutputFactory.java

Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>
@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented May 6, 2020

/integrate

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

@openjdk openjdk bot commented May 6, 2020

@mcimadamore
Pushed as commit e5a42f2.

@mcimadamore mcimadamore deleted the mcimadamore:jextract+abireorg 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

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