8329850: [AIX] Allow loading of different members of same shared library archive #18676

JoKern65 · 2024-04-08T12:27:05Z

With JDK-8320890 we introduced the capability not to load shared libraries twice if the application wants to do that. Instead we just rise a ref counter. Unfortunately this also suppresses the loading of a second member of a shared library.
This fix introduces an additionally stored hash value for each loaded member and only suppresses duplicate loading if the same member is loaded twice.
If a shared library has no member a hash value of 0 is used to make the code orthogonal.

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-8329850: [AIX] Allow loading of different members of same shared library archive (Bug - P3)

Reviewers

Martin Doerr (@TheRealMDoerr - Reviewer)
Matthias Baesken (@MBaesken - Reviewer) ⚠️ Review applies to b1ec5ed3
Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18676

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-08T12:27:54Z

👋 Welcome back jkern! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-08T12:29:31Z

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

8329850: [AIX] Allow loading of different members of same shared library archive

Reviewed-by: mdoerr, mbaesken, stuefe

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

ffd850f: 8309881: Qualified name of a type element depends on its origin (source vs class)
377f2e5: 8329862: libjli GetApplicationHome cleanups and enhance jli tracing
006f090: 8331150: RISC-V: Fix "bad AD file" bug
a407dc9: 8327512: JShell does not work correctly when a class named Object or Throwable is defined
ea06129: 8322847: java.lang.classfile.BufWriter should specify @throws for its writeXXX methods
b3bcc49: 8330181: Move PcDesc cache from nmethod header
1c238d4: 8319990: Update CLDR to Version 45.0
ce9eac3: 8331113: createJMHBundle.sh support configurable maven repo mirror
5af6b45: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
4dfaa9b: 8329805: Deprecate for removal ObjectOutputStream.PutField.write
... and 263 more: https://git.openjdk.org/jdk/compare/be45de1f93e918f928c6bb8bebc3697487adf602...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, @MBaesken, @tstuefe) 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).

openjdk · 2024-04-08T12:30:04Z

@JoKern65 The following label will be automatically applied to this pull request:

hotspot-runtime

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

mlbridge · 2024-04-08T12:33:22Z

Webrevs

TheRealMDoerr · 2024-04-08T12:54:56Z

src/hotspot/os/aix/porting_aix.cpp

+      while (*substr) {
+        hash = (hash << 5) - hash + (unsigned long)*substr++;
+      }
+    }


Should work in practical cases, but what if a hash collision occurs?

How likely is a hash collision? My first thought was to store the member string in an additional array. But there is no length limitation. So I cannot just add an char array of e.g. 20 chars, but have to use a pointer to an external dynamically loaded memory holding the string. I came to the conclusion that a hash is much more elegant with a neglectable small risk of a hash collision.

I guess extremely unlikely. But if it happens, it will be an extremely annoying problem.
I'd probably use strdup, but let's hear what other reviewers think.

JoKern65 · 2024-04-15T09:21:43Z

@tstuefe Would you be so kind and please take a look?

TheRealMDoerr

LGTM. Thanks for fixing it and for the update!

MBaesken

Looks okay to me.

But I think it would be really beneficial to have a (jtreg) test for this.

tstuefe

Had a brief skim over this change.

This would be a lot simpler had we just used string comparison instead of using inodes for equality. Then, we could have just used file name plus member name as key in the table.

Remind me again, why do we compare inodes instead of just file names?

tstuefe · 2024-04-25T13:15:53Z

src/hotspot/os/aix/porting_aix.cpp

    unsigned i = 0;
    TableLocker lock;
    // check if library belonging to filename is already loaded.
    // If yes use stored handle from previous ::dlopen() and increase refcount
    for (i = 0; i < g_handletable_used; i++) {
      if ((p_handletable + i)->handle &&
          (p_handletable + i)->inode == libstat.st_ino &&
-          (p_handletable + i)->devid == libstat.st_dev) {
+          (p_handletable + i)->devid == libstat.st_dev &&
+          (((p_handletable + i)->member == member) ||


If this ever fires (wildly unlikely), its a bug. You are comparing a stack address with something that lives in C heap right now.

I discussed this with Martin. The meaning of the line is: Check if both are nullptr.
My original implementation was:
(((p_handletable + i)->member == nullptr && member == nullptr) ||
Martin favoured the shorter version above which will have the same result.

It is, to me, not obvious where the member is reset. We delete it, but don't set the pointer to NULL.So, its not obvious that entry->member could not point to a former stale c heap address.

In any case, I prefer the cleanly written out logic. The generated code will be the same anyway, so this brevity just obfuscates your intent and confuses casual readers.

Back to the roots.

I think that Thomas' point is that member = nullptr; is missing after the free operation. I agree with that.

tstuefe · 2024-04-25T13:22:13Z

One simplification, maybe: Is there some limitation as to the length of the member name, and if yes, reasonably short? If yes, just make the member name inline ion the table entry and save the malloc/strdup and free.

JoKern65 · 2024-04-25T13:25:55Z

One simplification, maybe: Is there some limitation as to the length of the member name, and if yes, reasonably short? If yes, just make the member name inline ion the table entry and save the malloc/strdup and free.

I marked this already (see above). Unfortunately there seems to be no length limitation.

TheRealMDoerr

"Back to the roots" doesn't address the missing member = nullptr;.

tstuefe

LGTM

JoKern65 · 2024-04-26T09:13:58Z

/integrate

openjdk · 2024-04-26T09:15:20Z

@JoKern65
Your change (at version 97ad367) is now ready to be sponsored by a Committer.

TheRealMDoerr · 2024-04-26T09:25:38Z

/sponsor

openjdk · 2024-04-26T09:26:20Z

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

ffd850f: 8309881: Qualified name of a type element depends on its origin (source vs class)
377f2e5: 8329862: libjli GetApplicationHome cleanups and enhance jli tracing
006f090: 8331150: RISC-V: Fix "bad AD file" bug
a407dc9: 8327512: JShell does not work correctly when a class named Object or Throwable is defined
ea06129: 8322847: java.lang.classfile.BufWriter should specify @throws for its writeXXX methods
b3bcc49: 8330181: Move PcDesc cache from nmethod header
1c238d4: 8319990: Update CLDR to Version 45.0
ce9eac3: 8331113: createJMHBundle.sh support configurable maven repo mirror
5af6b45: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
4dfaa9b: 8329805: Deprecate for removal ObjectOutputStream.PutField.write
... and 263 more: https://git.openjdk.org/jdk/compare/be45de1f93e918f928c6bb8bebc3697487adf602...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-26T09:26:32Z

@TheRealMDoerr @JoKern65 Pushed as commit cfd19f0.

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

JDK-8329850

de6897e

openjdk bot added the rfr Pull request is ready for review label Apr 8, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Apr 8, 2024

JoKern65 mentioned this pull request Apr 8, 2024

8319516: AIX System::loadLibrary needs support to load a shared library from an archive object #17945

Closed

3 tasks

TheRealMDoerr reviewed Apr 8, 2024

View reviewed changes

openjdk bot changed the title ~~JDK-8329850: [AIX] Allow loading of different members of same shared library archive~~ 8329850: [AIX] Allow loading of different members of same shared library archive Apr 10, 2024

change from hash to allocated string

3ec7964

openjdk bot removed the rfr Pull request is ready for review label Apr 25, 2024

cosmetic changes

b1ec5ed

openjdk bot added the rfr Pull request is ready for review label Apr 25, 2024

TheRealMDoerr approved these changes Apr 25, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 25, 2024

MBaesken approved these changes Apr 25, 2024

View reviewed changes

tstuefe suggested changes Apr 25, 2024

View reviewed changes

Back to the roots

584502a

TheRealMDoerr suggested changes Apr 25, 2024

View reviewed changes

set ptr to nullptr after free

97ad367

TheRealMDoerr approved these changes Apr 25, 2024

View reviewed changes

tstuefe approved these changes Apr 26, 2024

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 26, 2024

openjdk bot added the integrated Pull request has been integrated label Apr 26, 2024

openjdk bot closed this Apr 26, 2024

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 Apr 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8329850: [AIX] Allow loading of different members of same shared library archive #18676

8329850: [AIX] Allow loading of different members of same shared library archive #18676

JoKern65 commented Apr 8, 2024 •

edited by openjdk bot

bridgekeeper bot commented Apr 8, 2024

openjdk bot commented Apr 8, 2024 •

edited

openjdk bot commented Apr 8, 2024

mlbridge bot commented Apr 8, 2024 •

edited

TheRealMDoerr Apr 8, 2024

JoKern65 Apr 8, 2024

TheRealMDoerr Apr 9, 2024

JoKern65 commented Apr 15, 2024

TheRealMDoerr left a comment

MBaesken left a comment

tstuefe left a comment

tstuefe Apr 25, 2024

JoKern65 Apr 25, 2024

tstuefe Apr 25, 2024

JoKern65 Apr 25, 2024

TheRealMDoerr Apr 25, 2024

tstuefe commented Apr 25, 2024

JoKern65 commented Apr 25, 2024

TheRealMDoerr left a comment

tstuefe left a comment

JoKern65 commented Apr 26, 2024

openjdk bot commented Apr 26, 2024

TheRealMDoerr commented Apr 26, 2024

openjdk bot commented Apr 26, 2024

openjdk bot commented Apr 26, 2024

8329850: [AIX] Allow loading of different members of same shared library archive #18676

8329850: [AIX] Allow loading of different members of same shared library archive #18676

Conversation

JoKern65 commented Apr 8, 2024 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 8, 2024

openjdk bot commented Apr 8, 2024 • edited

openjdk bot commented Apr 8, 2024

mlbridge bot commented Apr 8, 2024 • edited

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoKern65 commented Apr 15, 2024

TheRealMDoerr left a comment

Choose a reason for hiding this comment

MBaesken left a comment

Choose a reason for hiding this comment

tstuefe left a 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

Choose a reason for hiding this comment

tstuefe commented Apr 25, 2024

JoKern65 commented Apr 25, 2024

TheRealMDoerr left a comment

Choose a reason for hiding this comment

tstuefe left a comment

Choose a reason for hiding this comment

JoKern65 commented Apr 26, 2024

openjdk bot commented Apr 26, 2024

TheRealMDoerr commented Apr 26, 2024

openjdk bot commented Apr 26, 2024

openjdk bot commented Apr 26, 2024

JoKern65 commented Apr 8, 2024 •

edited by openjdk bot

openjdk bot commented Apr 8, 2024 •

edited

mlbridge bot commented Apr 8, 2024 •

edited