JDK-8320005 : Allow loading of shared objects with .a extension on AIX #16604

suchismith1993 · 2023-11-10T12:28:46Z

J2SE agent does not start and throws error when it tries to find the shared library ibm_16_am.
After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load fails.It fails to identify the shared library ibm_16_am.a shared archive file on AIX.
Hence we are providing a function which will additionally search for .a file on AIX ,when the search for .so file fails.

Progress

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

Issue

JDK-8320005: Allow loading of shared objects with .a extension on AIX (Bug - P4)

Reviewers

Amit Kumar (@offamitkumar - Committer) ⚠️ Review applies to c6332f67
Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to 1943445d
Joachim Kern (@JoKern65 - Author) ⚠️ Review applies to c6332f67
Martin Doerr (@TheRealMDoerr - Reviewer) ⚠️ Review applies to c6332f67

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16604

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-10T12:30:32Z

👋 Welcome back suchismith1993! 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 · 2023-11-10T12:31:51Z

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

hotspot
serviceability

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.

openjdk · 2023-11-22T13:37:28Z

@suchismith1993 This pull request has not yet been marked as ready for integration.

mlbridge · 2023-11-22T16:19:29Z

Webrevs

tstuefe · 2023-11-22T16:35:36Z

Hi, is this patch meant for review already? If yes, could you please describe the problem you fix, and how you fix it? If no, I suggest working on it in draft state till its ready for review.

suchismith1993 · 2023-11-23T05:24:07Z

I have updated the description. Let me know if anything is missing

offamitkumar

some nits you might want to consider.

offamitkumar · 2023-11-23T05:42:40Z

src/hotspot/os/aix/os_aix.cpp

+//Replaces provided path with alternate path for the given file,if it doesnt exist.
+//For AIX,this replaces .so with .a.


Suggested change

//Replaces provided path with alternate path for the given file,if it doesnt exist.

//For AIX,this replaces .so with .a.

// Replaces the specified path with an alternative path for the given file if the original path doesn't exist.

// For AIX, this replaces extension from ".so" to ".a".

offamitkumar · 2023-11-23T05:43:14Z

src/hotspot/os/aix/os_aix.cpp

+
+//Replaces provided path with alternate path for the given file,if it doesnt exist.
+//For AIX,this replaces .so with .a.
+void os::Aix::mapAlternateName(char* buffer, const char *extension) {


Suggested change

void os::Aix::mapAlternateName(char* buffer, const char *extension) {

void os::Aix::map_alternate_name(char* buffer, const char *extension) {

offamitkumar · 2023-11-23T05:44:03Z

src/hotspot/os/aix/os_aix.hpp

+  // Provide alternate path name,if file does not exist.
+  static void mapAlternateName(char* buffer, const char *extension);


Suggested change

// Provide alternate path name,if file does not exist.

static void mapAlternateName(char* buffer, const char *extension);

// Provides alternate path name, if file does not exist.

static void map_alternate_name(char* buffer, const char *extension);

offamitkumar · 2023-11-23T05:49:41Z

Also are you planning to close this one : #16490 ? JBS issue is already closed.

dholmes-ora

A couple of comments.

First, if you always want to look for libfoo.a if you can't find libfoo.so then maybe that should be handled as the os::dll_load level? Otherwisae it is not clear why this is something you only do for agents. ?/

Second, the amount of AIX-specific code in the shared jvmtiAgent.cpp now seems unreasonable. As I think @tstuefe mentioned in another PR perhaps it is time to find better abstractions here to hide all these AIX specific quirks?

dholmes-ora · 2023-11-23T05:56:52Z

src/hotspot/os/aix/os_aix.cpp

+    end = end - 1;
+  }
+  buffer[end] = '\0';
+  strcat(buffer, extension);


At some point you need to check the length of extension won't overflow buffer.

tstuefe

I'm not a big fan of this approach. We accumulate more and more "#ifdef AIX" in shared code because of many recent AIX additions. No other platform has such a large ifdef footprint in shared code.

I argue that all of this should be handled inside os_aix.cpp and not leak out into the external space:

If .a is a valid shared object format on AIX, this should be handled in os::dll_load(), and be done for all shared objects. If not, why do we try to load a static archive via dlload in this case but not in other cases?

If this is needed in shared code, the string replacement function should be a generic utility function for all platforms, and it should be tested with a small gtest. A gtest would have likely uncovered the buffer overflow too.

tstuefe · 2023-11-23T05:55:28Z

src/hotspot/os/aix/os_aix.cpp

+  unsigned long end = strlen(buffer);
+  while (end > 0 && buffer[end] != '.') {
+    end = end - 1;
+  }


Use strrchr.

Pls handle the case where the string contains no dot.

tstuefe · 2023-11-23T05:56:37Z

src/hotspot/os/aix/os_aix.cpp

+  }
+  buffer[end] = '\0';
+  strcat(buffer, extension);
+ }


This is a buffer overrun waiting to happen if replacement is larger than original extension.

src/hotspot/os/aix/os_aix.cpp

suchismith1993 · 2023-11-23T08:08:51Z

The JBS issue with respect to that has been closed. Need to check if that PR is required. Currently putting it on hold.

offamitkumar · 2023-11-23T08:50:39Z

This response on the issue suggest otherwise:

<S12Y triage team>: The JDK does not support dynamically loaded archive files (.a files) and there are no plans to add this support. Closing as will not fix

suchismith1993 · 2023-11-23T09:48:11Z

In AIX, we have shared objects with .a extension and also static archives with .a extension.I think in other platforms the format for shared objects is fixed? . In that case does this become specific to AIX?

openjdk · 2024-02-27T08:19:01Z

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

TheRealMDoerr · 2024-02-27T08:22:21Z

src/hotspot/os/aix/os_aix.cpp

+  // Shared object in .so format dont have braces, hence they get removed for archives with members.
+  if (result == nullptr && pointer_to_dot != nullptr && strcmp(pointer_to_dot, old_extension) == 0) {
+      snprintf(pointer_to_dot, sizeof(old_extension), "%s", new_extension);
+      result = dll_load_library(file_path, ebuf, ebuflen);


You should have adapted the indentation, too.

Which indentation ?

oh! to reduce the spaces

@TheRealMDoerr adapted

suchismith1993 · 2024-02-27T08:51:53Z

/integrate

openjdk · 2024-02-27T08:53:46Z

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

TheRealMDoerr · 2024-02-27T09:13:33Z

/sponsor

openjdk · 2024-02-27T09:15:41Z

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

5d3d40d: 8326659: Serial: Remove redundant TenuredSpace::print_on
7583419: 8324828: Avoid multiple search of reserved regions during splitting
c5f1dcc: 8325082: Rename headers named 'heapRegion*' of G1
4d82ea1: 8325144: C1: Optimize CriticalEdgeFinder
a48f596: 8322179: RISC-V: Implement SHA-1 intrinsic
5daf63b: 8326636: Problem list StartupOutput.java due to 8326615
97aca09: 8326717: Disable stringop-overflow in shenandoahLock.cpp
ec9437d: 8325247: Memory leak in SessionKeyRef class def when using PKCS11 security provider
0901ded: 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c
d22d890: 8325898: ChoiceFormat returns erroneous result when formatting bad pattern
... and 499 more: https://git.openjdk.org/jdk/compare/36f4b34f1953af736706ec67192204727808bc6c...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-27T09:15:49Z

@TheRealMDoerr @suchismith1993 Pushed as commit e85355a.

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

suchismith1993 · 2024-03-05T08:14:24Z

/backport jdk21u

openjdk · 2024-03-05T08:15:24Z

@suchismith1993 the backport was successfully created on the branch backport-suchismith1993-e85355ad in my personal fork of openjdk/jdk21u. To create a pull request with this backport targeting openjdk/jdk21u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u:

$ git fetch https://github.com/openjdk-bots/jdk21u.git backport-suchismith1993-e85355ad:backport-suchismith1993-e85355ad
$ git checkout backport-suchismith1993-e85355ad
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u.git backport-suchismith1993-e85355ad

⚠️ @suchismith1993 You are not yet a collaborator in my fork openjdk-bots/jdk21u. An invite will be sent out and you need to accept it before you can proceed.

TheRealMDoerr · 2024-03-05T08:48:15Z

jdk21u is only open for critical backports and requires special approval. Please backport it to jdk21u-dev.
What about jdk22u?
/backport commands should be used in commits, not in closed PRs.
Not sure if it works without Committer privileges.

magicus · 2024-03-05T10:52:20Z

@TheRealMDoerr FYI, /backport works in PRs as well as commits since some time ago... In fact, I would recommend it as the better way to create a backport, now that it exists.

suchismith1993 · 2024-03-06T07:56:36Z

/backport jdk22u

openjdk · 2024-03-06T07:57:38Z

@suchismith1993 the backport was successfully created on the branch backport-suchismith1993-e85355ad in my personal fork of openjdk/jdk22u. To create a pull request with this backport targeting openjdk/jdk22u:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk22u:

$ git fetch https://github.com/openjdk-bots/jdk22u.git backport-suchismith1993-e85355ad:backport-suchismith1993-e85355ad
$ git checkout backport-suchismith1993-e85355ad
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk22u.git backport-suchismith1993-e85355ad

⚠️ @suchismith1993 You are not yet a collaborator in my fork openjdk-bots/jdk22u. An invite will be sent out and you need to accept it before you can proceed.

suchismith1993 · 2024-03-06T08:41:03Z

/backport jdk21u-dev

suchismith1993 · 2024-03-06T08:41:41Z

/backport jdk11u-dev

suchismith1993 · 2024-03-06T08:41:52Z

/backport jdk17u-dev

suchismith1993 · 2024-03-06T08:42:00Z

/backport jdk8u-dev

openjdk · 2024-03-06T08:42:52Z

@suchismith1993 the backport was successfully created on the branch backport-suchismith1993-e85355ad in my personal fork of openjdk/jdk21u-dev. To create a pull request with this backport targeting openjdk/jdk21u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk21u-dev:

$ git fetch https://github.com/openjdk-bots/jdk21u-dev.git backport-suchismith1993-e85355ad:backport-suchismith1993-e85355ad
$ git checkout backport-suchismith1993-e85355ad
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk21u-dev.git backport-suchismith1993-e85355ad

⚠️ @suchismith1993 You are not yet a collaborator in my fork openjdk-bots/jdk21u-dev. An invite will be sent out and you need to accept it before you can proceed.

openjdk · 2024-03-06T08:44:34Z

@suchismith1993 Could not automatically backport e85355ad to openjdk/jdk11u-dev due to conflicts in the following files:

src/hotspot/os/aix/os_aix.cpp

Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk11u-dev. Note: these commands are just some suggestions and you can use other equivalent commands you know.

# Fetch the up-to-date version of the target branch
$ git fetch --no-tags https://git.openjdk.org/jdk11u-dev.git master:master

# Check out the target branch and create your own branch to backport
$ git checkout master
$ git checkout -b backport-suchismith1993-e85355ad

# Fetch the commit you want to backport
$ git fetch --no-tags https://git.openjdk.org/jdk.git e85355ada4ac1061c49ee9f1247d37a437c7b5ab

# Backport the commit
$ git cherry-pick --no-commit e85355ada4ac1061c49ee9f1247d37a437c7b5ab
# Resolve conflicts now

# Commit the files you have modified
$ git add files/with/resolved/conflicts
$ git commit -m 'Backport e85355ada4ac1061c49ee9f1247d37a437c7b5ab'

Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk11u-dev with the title Backport e85355ada4ac1061c49ee9f1247d37a437c7b5ab.

Below you can find a suggestion for the pull request body:

openjdk · 2024-03-06T08:45:57Z

@suchismith1993 Could not automatically backport e85355ad to openjdk/jdk17u-dev due to conflicts in the following files:

src/hotspot/os/aix/os_aix.cpp

Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk17u-dev. Note: these commands are just some suggestions and you can use other equivalent commands you know.

# Fetch the up-to-date version of the target branch
$ git fetch --no-tags https://git.openjdk.org/jdk17u-dev.git master:master

# Check out the target branch and create your own branch to backport
$ git checkout master
$ git checkout -b backport-suchismith1993-e85355ad

# Fetch the commit you want to backport
$ git fetch --no-tags https://git.openjdk.org/jdk.git e85355ada4ac1061c49ee9f1247d37a437c7b5ab

# Backport the commit
$ git cherry-pick --no-commit e85355ada4ac1061c49ee9f1247d37a437c7b5ab
# Resolve conflicts now

# Commit the files you have modified
$ git add files/with/resolved/conflicts
$ git commit -m 'Backport e85355ada4ac1061c49ee9f1247d37a437c7b5ab'

Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk17u-dev with the title Backport e85355ada4ac1061c49ee9f1247d37a437c7b5ab.

Below you can find a suggestion for the pull request body:

openjdk · 2024-03-06T08:47:25Z

@suchismith1993 Could not automatically backport e85355ad to openjdk/jdk8u-dev due to conflicts in the following files:

src/hotspot/os/aix/os_aix.cpp

Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk8u-dev. Note: these commands are just some suggestions and you can use other equivalent commands you know.

# Fetch the up-to-date version of the target branch
$ git fetch --no-tags https://git.openjdk.org/jdk8u-dev.git master:master

# Check out the target branch and create your own branch to backport
$ git checkout master
$ git checkout -b backport-suchismith1993-e85355ad

# Fetch the commit you want to backport
$ git fetch --no-tags https://git.openjdk.org/jdk.git e85355ada4ac1061c49ee9f1247d37a437c7b5ab

# Backport the commit
$ git cherry-pick --no-commit e85355ada4ac1061c49ee9f1247d37a437c7b5ab
# Resolve conflicts now

# Commit the files you have modified
$ git add files/with/resolved/conflicts
$ git commit -m 'Backport e85355ada4ac1061c49ee9f1247d37a437c7b5ab'

Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk8u-dev with the title Backport e85355ada4ac1061c49ee9f1247d37a437c7b5ab.

Below you can find a suggestion for the pull request body:

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Nov 10, 2023

suchismith1993 changed the title ~~Add support for .a extension in jvm agent.~~ JDK-8320005 : Add support for .a extension in jvm agent. Nov 14, 2023

suchismith1993 force-pushed the jvmagent branch from def0e2f to 58f14cc Compare November 14, 2023 08:15

suchismith1993 changed the title ~~JDK-8320005 : Add support for .a extension in jvm agent.~~ JDK-8320005 : Native library suffix impact on hotspot code in AIX. Nov 14, 2023

suchismith1993 force-pushed the jvmagent branch 8 times, most recently from dd0bf17 to 530c45c Compare November 20, 2023 08:05

suchismith1993 force-pushed the jvmagent branch from 530c45c to d8ea63e Compare November 22, 2023 13:52

suchismith1993 changed the title ~~JDK-8320005 : Native library suffix impact on hotspot code in AIX.~~ JDK-8320005 : Native library suffix impact on hotspot code in AIX Nov 22, 2023

suchismith1993 marked this pull request as ready for review November 22, 2023 16:15

openjdk bot added the rfr Pull request is ready for review label Nov 22, 2023

offamitkumar suggested changes Nov 23, 2023

View reviewed changes

dholmes-ora reviewed Nov 23, 2023

View reviewed changes

tstuefe suggested changes Nov 23, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 27, 2024

TheRealMDoerr reviewed Feb 27, 2024

View reviewed changes

openjdk bot removed the sponsor Pull request is ready to be sponsored label Feb 27, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 27, 2024

openjdk bot added the integrated Pull request has been integrated label Feb 27, 2024

openjdk bot closed this Feb 27, 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 Feb 27, 2024

suchismith1993 mentioned this pull request Jun 30, 2024

JDK-8334217 : [AIX] Misleading error messages after JDK-8320005 #19887

Closed

3 tasks

		//Replaces provided path with alternate path for the given file,if it doesnt exist.
		//For AIX,this replaces .so with .a.

	void os::Aix::mapAlternateName(char* buffer, const char *extension) {
	void os::Aix::map_alternate_name(char* buffer, const char *extension) {

		// Provide alternate path name,if file does not exist.
		static void mapAlternateName(char* buffer, const char *extension);

JDK-8320005 : Allow loading of shared objects with .a extension on AIX #16604

JDK-8320005 : Allow loading of shared objects with .a extension on AIX #16604

Conversation

suchismith1993 commented Nov 10, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Nov 10, 2023

openjdk bot commented Nov 10, 2023

openjdk bot commented Nov 22, 2023

mlbridge bot commented Nov 22, 2023 • edited Loading

Webrevs

tstuefe commented Nov 22, 2023

suchismith1993 commented Nov 23, 2023

offamitkumar 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

offamitkumar commented Nov 23, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tstuefe left a comment • edited Loading

Choose a reason for hiding this comment

tstuefe Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

tstuefe Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suchismith1993 commented Nov 23, 2023

offamitkumar commented Nov 23, 2023

suchismith1993 commented Nov 23, 2023

openjdk bot commented Feb 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suchismith1993 Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

suchismith1993 commented Feb 27, 2024

openjdk bot commented Feb 27, 2024

TheRealMDoerr commented Feb 27, 2024

openjdk bot commented Feb 27, 2024

openjdk bot commented Feb 27, 2024

suchismith1993 commented Mar 5, 2024

openjdk bot commented Mar 5, 2024

TheRealMDoerr commented Mar 5, 2024 • edited Loading

magicus commented Mar 5, 2024

suchismith1993 commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

suchismith1993 commented Mar 6, 2024

suchismith1993 commented Mar 6, 2024

suchismith1993 commented Mar 6, 2024

suchismith1993 commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

suchismith1993 commented Nov 10, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 22, 2023 •

edited

Loading

tstuefe left a comment •

edited

Loading

tstuefe Nov 23, 2023 •

edited

Loading

tstuefe Nov 23, 2023 •

edited

Loading

suchismith1993 Feb 27, 2024 •

edited

Loading

TheRealMDoerr commented Mar 5, 2024 •

edited

Loading