JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols #16414

suchismith1993 · 2023-10-30T10:54:48Z

The math library in AIX specifically, is a static archive. Doing a -lm wont suffice, because when the symbols are looked up using dlsym or accessing native code through Java, it will lead to failures.
Hence we had to come up with a list of symbols to allow math library symbols to be accesible.
Also, there are parts of libc library that are static too, and hence those symbols also are present in this list.
Without this change, the StdLibTest and multiple other tests which make native function calls using FFI, fail with NoSuchElementException.

Adding required compiler flags.
Adding required symbols.

JBS-ISSUE : JDK-8317799

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8317799: AIX PPC64: FFI symbol lookup doesn't find symbols (Sub-task - P3)

Reviewers

Martin Doerr (@TheRealMDoerr - Reviewer) ⚠️ Review applies to efbb245a
Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to efbb245a
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to efbb245a
Jorn Vernee (@JornVernee - Reviewer) ⚠️ Review applies to efbb245a

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16414/head:pull/16414
$ git checkout pull/16414

Update a local copy of the PR:
$ git checkout pull/16414
$ git pull https://git.openjdk.org/jdk.git pull/16414/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16414

View PR using the GUI difftool:
$ git pr show -t 16414

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16414.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-10-30T10:56:32Z

Hi @suchismith1993, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user suchismith1993" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2023-10-30T10:58:20Z

@suchismith1993 The following labels will be automatically applied to this pull request:

build
core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

suchismith1993 · 2023-10-30T12:58:27Z

/covered

bridgekeeper · 2023-10-30T13:00:00Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

TheRealMDoerr · 2023-10-31T10:12:03Z

make/modules/java.base/Lib.gmk

@@ -213,8 +213,10 @@ $(eval $(call SetupJdkLibrary, BUILD_SYSLOOKUPLIB, \
    CXXFLAGS := $(CXXFLAGS_JDKLIB), \
    LDFLAGS := $(LDFLAGS_JDKLIB), \
    LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \
+    LDFLAGS_aix := -brtl  -bloadmap:/home/hotspot/openjdk/symbol.log -bE:/home/hotspot/tmp/all-libs/1.exp   , \


These files need to get added somewhere. Maybe symbols could get added to make/data/hotspot-symbols/symbols-aix? Or to new files in the same directory (with aix in the file names)?
Also, please remove extra whitespaces.

TheRealMDoerr · 2023-10-31T10:14:00Z

make/modules/java.base/Lib.gmk

-  TARGETS += $(BUILD_LIB_SIMD_SORT)
-endif
-
-################################################################################


I guess this was done by mistake. Please revert.

TheRealMDoerr · 2023-10-31T10:15:44Z

src/java.base/share/native/libsyslookup/syslookup.c

+
+
+
+


Please avoid changing lines which you don't need to modify! Is #include <math.h> really needed, here? If so, please protect it by #ifdef _AIX and add a comment explaining why.

TheRealMDoerr

LGTM. You may want to replace the Copyright header of the new file. It was contributed by IBM.

TheRealMDoerr

Still good. I suggest to remove the empty lines at the beginning.

openjdk · 2023-11-08T20:49:51Z

@suchismith1993 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:

8317799: AIX PPC64: FFI symbol lookup doesn't find symbols

Reviewed-by: mdoerr, ihse, dholmes, jvernee

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 21 new commits pushed to the master branch:

c39d001: 8319137: release _object in ObjectMonitor dtor to avoid races
8b47a14: 8320309: AIX: pthreads created by foreign test library don't work as expected
bf0a904: 8318175: AIX PPC64: Handle alignment of double in structs
6ce0ebb: 8295343: sun/security/pkcs11 tests fail on Linux RHEL 8.6 and newer
5e81831: 8320399: RISC-V: Some format clean-up in opto assembly code
ceb0290: 8320447: Remove obsolete LintCategory.hidden
98edb03: 8320382: Remove CompressedKlassPointers::is_valid_base()
2c31ca5: 8320335: Deprecate RegisterFinalizersAtInit option and code
b3616c9: 8320131: Zero build fails on macOS after JDK-8254693
d1af748: 8309067: gtest/AsyncLogGtest.java fails again in stderrOutput_vm
... and 11 more: https://git.openjdk.org/jdk/compare/923207073af985a1b72de3c777d55b0c2d392b25...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TheRealMDoerr, @magicus, @dholmes-ora, @JornVernee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

mlbridge · 2023-11-08T20:53:20Z

Webrevs

magicus · 2023-11-08T21:08:43Z

No, no... This is bad on several accounts.

magicus · 2023-11-08T21:11:32Z

First and foremost, the make/data/hotspot-symbols directory is, as the name says, for hotspot symbols. These are not hotspot symbols. If you really do need the file, then it needs to reside in a proper location.

Secondly: do you really need this file? It looks just like an enumeration of (a subset of?) standard library functions. This looks like a completely incorrect solution to the problem. How did you arrive at this list? What if in the future new functions are added to the standard library?

magicus · 2023-11-08T21:14:41Z

Also, please don't ever force push once you have published a PR. Now it makes Martin's comment just dangling in the air.

magicus · 2023-11-21T13:46:08Z

src/java.base/aix/native/libsyslookup/syslookup.c

+#include <stdlib.h>
+#include <math.h>
+
+// Simple dummy function so this library appears as a normal library to tooling.


I assume this comment should be on line 207 instead..?

magicus · 2023-11-21T13:46:35Z

This looks like a much saner solution than the original attempt.

TheRealMDoerr · 2023-11-21T13:48:15Z

src/java.base/aix/native/libsyslookup/syslookup.c

+
+char* syslookup() {
+  return "syslookup";
+}


This is probably not needed, either (see Windows version).

TheRealMDoerr · 2023-11-21T17:50:04Z

src/java.base/aix/native/libsyslookup/syslookup.c

+// Adding at least one #include removes unwanted warnings on some platforms.
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>


Are string.h and stdlib.h needed? I can't see them in the comments below.

string.h is needed for strlen. Let me check for stdlib.h by excluding it.

ah, a comment above strlen would help.

TheRealMDoerr · 2023-11-21T17:52:39Z

src/java.base/aix/native/libsyslookup/syslookup.c

+#include <stdlib.h>
+#include <math.h>
+
+// Addresses of functions to referenced using static linking.


What does "functions to referenced" mean? Typo?

To be Referenced*. Is that the right comment ?

TheRealMDoerr

Thanks for cleaning it up! LGTM.

dholmes-ora

This is much more palatable and good to be consistent with what is done on Windows. Thank.

JornVernee · 2023-11-22T06:50:07Z

Note that on Windows we also have a lookup mechanism on the Java side:

jdk/src/java.base/share/classes/jdk/internal/foreign/SystemLookup.java

Line 159 in 2c31ca5

private enum WindowsFallbackSymbols {

On Windows we need to load the global array, and then grab functions from the array. Why isn't that needed on AIX? Is dlsym able to find the statically linked functions as well?

suchismith1993 · 2023-11-22T07:29:43Z

Yes.The test cases for lookup of symbols have passed. I think the bexpfull option helps in exporting the right symbols being referenced.

…

On Wednesday, November 22, 2023, Jorn Vernee ***@***.***> wrote: Note that on Windows we also have a lookup mechanism on the Java side: https://github.com/openjdk/jdk/blob/2c31ca525b1cd70c3dfcb0463c8c98 4bdd7c886a/src/java.base/share/classes/jdk/internal/ foreign/SystemLookup.java#L159 On Windows we need to load the global array, and then grab functions from the array. Why isn't that needed on AIX? Is dlsym able to find the statically linked functions as well? — Reply to this email directly, view it on GitHub <#16414 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AESW3BAJPWKB5I7MM7IVQKDYFWOCTAVCNFSM6AAAAAA6VZ2E4KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRSGIYDKNBUGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

src/java.base/aix/native/libsyslookup/syslookup.c

Co-authored-by: Amit Kumar <amitlocham09@icloud.com>

suchismith1993 · 2023-11-22T13:33:21Z

Thank you @JornVernee @magicus @dholmes-ora @TheRealMDoerr @erikj79
Thanks for enocuraging to work towards a better solution which enabled me to understand and explore windows code as well.

suchismith1993 · 2023-11-22T15:05:06Z

/integrate

openjdk · 2023-11-22T15:06:19Z

@suchismith1993
Your change (at version 7a68313) is now ready to be sponsored by a Committer.

offamitkumar · 2023-11-22T15:42:21Z

/sponsor

openjdk · 2023-11-22T15:43:37Z

Going to push as commit 25cebe8.
Since your change was applied there have been 21 commits pushed to the master branch:

c39d001: 8319137: release _object in ObjectMonitor dtor to avoid races
8b47a14: 8320309: AIX: pthreads created by foreign test library don't work as expected
bf0a904: 8318175: AIX PPC64: Handle alignment of double in structs
6ce0ebb: 8295343: sun/security/pkcs11 tests fail on Linux RHEL 8.6 and newer
5e81831: 8320399: RISC-V: Some format clean-up in opto assembly code
ceb0290: 8320447: Remove obsolete LintCategory.hidden
98edb03: 8320382: Remove CompressedKlassPointers::is_valid_base()
2c31ca5: 8320335: Deprecate RegisterFinalizersAtInit option and code
b3616c9: 8320131: Zero build fails on macOS after JDK-8254693
d1af748: 8309067: gtest/AsyncLogGtest.java fails again in stderrOutput_vm
... and 11 more: https://git.openjdk.org/jdk/compare/923207073af985a1b72de3c777d55b0c2d392b25...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-22T15:43:44Z

@offamitkumar @suchismith1993 Pushed as commit 25cebe8.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

bridgekeeper bot added the oca Needs verification of OCA signatory status label Oct 30, 2023

openjdk bot added build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Oct 30, 2023

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Oct 30, 2023

TheRealMDoerr suggested changes Oct 31, 2023

View reviewed changes

suchismith1993 force-pushed the pull/16179 branch 9 times, most recently from 07effd7 to 4533b71 Compare November 6, 2023 17:31

TheRealMDoerr approved these changes Nov 6, 2023

View reviewed changes

suchismith1993 marked this pull request as ready for review November 8, 2023 16:18

TheRealMDoerr approved these changes Nov 8, 2023

View reviewed changes

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Nov 8, 2023

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 8, 2023

magicus reviewed Nov 21, 2023

View reviewed changes

TheRealMDoerr reviewed Nov 21, 2023

View reviewed changes

suchismith1993 added 4 commits November 21, 2023 17:33

Change comments

d150525

Comments

48a69c5

Remove unnecessary includes

2383be7

Fix Typos

efbb245

TheRealMDoerr reviewed Nov 21, 2023

View reviewed changes

TheRealMDoerr approved these changes Nov 21, 2023

View reviewed changes

magicus approved these changes Nov 21, 2023

View reviewed changes

dholmes-ora approved these changes Nov 22, 2023

View reviewed changes

JornVernee approved these changes Nov 22, 2023

View reviewed changes

varada1110 mentioned this pull request Nov 22, 2023

JDK-8320309: AIX: pthreads created by foreign test library don't work as expected #16757

Closed

3 tasks

offamitkumar reviewed Nov 22, 2023

View reviewed changes

src/java.base/aix/native/libsyslookup/syslookup.c Outdated Show resolved Hide resolved

Update src/java.base/aix/native/libsyslookup/syslookup.c copyright

7a68313

Co-authored-by: Amit Kumar <amitlocham09@icloud.com>

openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 22, 2023

openjdk bot added the integrated Pull request has been integrated label Nov 22, 2023

openjdk bot closed this Nov 22, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols #16414

JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols #16414

suchismith1993 commented Oct 30, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

suchismith1993 commented Oct 30, 2023

bridgekeeper bot commented Oct 30, 2023

TheRealMDoerr Oct 31, 2023

TheRealMDoerr Oct 31, 2023

TheRealMDoerr Oct 31, 2023

TheRealMDoerr left a comment

TheRealMDoerr left a comment

openjdk bot commented Nov 8, 2023 •

edited

Loading

mlbridge bot commented Nov 8, 2023 •

edited

Loading

magicus commented Nov 8, 2023

magicus commented Nov 8, 2023

magicus commented Nov 8, 2023

magicus Nov 21, 2023

magicus commented Nov 21, 2023

TheRealMDoerr Nov 21, 2023

TheRealMDoerr Nov 21, 2023

suchismith1993 Nov 21, 2023

TheRealMDoerr Nov 21, 2023

TheRealMDoerr Nov 21, 2023

suchismith1993 Nov 21, 2023

TheRealMDoerr left a comment

dholmes-ora left a comment

JornVernee commented Nov 22, 2023

suchismith1993 commented Nov 22, 2023 via email

suchismith1993 commented Nov 22, 2023

suchismith1993 commented Nov 22, 2023

openjdk bot commented Nov 22, 2023

offamitkumar commented Nov 22, 2023

openjdk bot commented Nov 22, 2023

openjdk bot commented Nov 22, 2023

JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols #16414

JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols #16414

Conversation

suchismith1993 commented Oct 30, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 30, 2023

openjdk bot commented Oct 30, 2023

suchismith1993 commented Oct 30, 2023

bridgekeeper bot commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealMDoerr left a comment

Choose a reason for hiding this comment

TheRealMDoerr left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 8, 2023 • edited Loading

mlbridge bot commented Nov 8, 2023 • edited Loading

Webrevs

magicus commented Nov 8, 2023

magicus commented Nov 8, 2023

magicus commented Nov 8, 2023

Choose a reason for hiding this comment

magicus commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealMDoerr left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

JornVernee commented Nov 22, 2023

suchismith1993 commented Nov 22, 2023 via email

suchismith1993 commented Nov 22, 2023

suchismith1993 commented Nov 22, 2023

openjdk bot commented Nov 22, 2023

offamitkumar commented Nov 22, 2023

openjdk bot commented Nov 22, 2023

openjdk bot commented Nov 22, 2023

suchismith1993 commented Oct 30, 2023 •

edited by openjdk bot

Loading

openjdk bot commented Nov 8, 2023 •

edited

Loading

mlbridge bot commented Nov 8, 2023 •

edited

Loading