-
Notifications
You must be signed in to change notification settings - Fork 5.8k
8304888: Add dedicated VMProps for linker and fallback linker #13429
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
Conversation
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>
Fix ULE when intializing LibFallback
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
Co-authored-by: Maurizio Cimadamore <54672762+mcimadamore@users.noreply.github.com>
The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout VMProps
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push |
@JornVernee this pull request can not be integrated into git checkout VMProps
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
|
@JornVernee This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 15 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Thanks for the reveiw. I've added a comment that explains the property. During testing after the merge I noticed that the I've fixed this and re-tested on the following configurations:
For the latter two to pass, I had to fix an issue in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the update (comment on VMProps. cabi, update to TestLinker, and s/UNKNOWN/UnSUPPORTED). Looks okay to me.
@@ -24,7 +24,7 @@ | |||
/* | |||
* @test | |||
* @enablePreview | |||
* @requires ((os.arch == "amd64" | os.arch == "x86_64") & sun.arch.data.model == "64") | os.arch == "aarch64" | os.arch == "riscv64" | |||
* @requires jdk.CABI != "UNSUPPORTED" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the name jdk.native.abi
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so sure. The CABI is really the linker kind, not the underlying ABI ('CABI' is probably a misnomer at this point, since we no longer use it for anything else besides selecting the linker implementation). It can also be FALLBACK
which is not an ABI.
If CABI is bad, I think jdk.foreign.linker
would be better as a property name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - I'm not too much of a fan of "CABI" as a property name - but that's a minor nit. I think the improvements in the jtreg test headers are very nice.
I went with the |
Looks good (if we wanted to be 100% explicit, then it would have been jdk.foreign.system.linker/jdk.foreign.native.linker - but what you picked is better than what it replaces). |
/integrate |
Going to push as commit a8bf2ac.
Your commit was automatically rebased without conflicts. |
@JornVernee Pushed as commit a8bf2ac. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This patch adds a dedicated jtreg property to check which CABI is being used by the JDK, which can be used both to check whether the foreign linker is supported, and whether the fallback linker is being used. (and, possibly it can also be use to check for a particular ABI in case we want to add ABI specific tests).
Checking whether the foreign linker is supported currently requires using an unwieldy expression that checks if we are running on a platform that has a foreign linker port. Checking for the fallback linker currently uses
vm.flavor != "zero"
which is not always correct, since the fallback linker can also be used on other platforms which are notzero
.To initialize the property, VMProps directly reads
jdk.internal.foreig.CABI::current()
. Since this class is in an internal package,--add-exports
flags are added as javac flags and VM flags for the extra prop definitions class.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/13429/head:pull/13429
$ git checkout pull/13429
Update a local copy of the PR:
$ git checkout pull/13429
$ git pull https://git.openjdk.org/jdk.git pull/13429/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 13429
View PR using the GUI difftool:
$ git pr show -t 13429
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13429.diff
Webrev
Link to Webrev Comment