RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v25]

[Thomas Stuefe]

On Tue, 20 Feb 2024 09:27:15 GMT, Suchismith Roy <sroy at openjdk.org> wrote:

>> 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.
>
> Suchismith Roy has updated the pull request incrementally with one additional commit since the last revision:
> 
>    remove space

Changes requested by stuefe (Reviewer).

src/hotspot/os/aix/os_aix.cpp line 1167:

> 1165:  Load "libfilename.so" first, then load libfilename.a, on failure.
> 1166:  In OpenJ9, the libary with .so extension is loaded first and then .a extension, on failure.
> 1167: */

Wrong block comment, but the comment itself is also off now. Suggestion:

Suggestion:

// Load library named <filename>
// If filename matches <name>.so, and loading fails, repeat with <name>.a.

src/hotspot/os/aix/os_aix.cpp line 1173:

> 1171:   char* const pointer_to_dot = strrchr(file_path, '.');
> 1172:   char const *old_extension = ".so";
> 1173:   char const *new_extension = ".a";

Suggestion:

  char* const file_path = strdup(filename);
  char* const pointer_to_dot = strrchr(file_path, '.');
  const char old_extension[] = ".so";
  const char new_extension[] = ".a";
  STATIC_ASSERT(sizeof(old_exception) >= sizeof(new_exception));

and remove runtime-assert below

src/hotspot/os/aix/os_aix.cpp line 1178:

> 1176:     FREE_C_HEAP_ARRAY(char, file_path);
> 1177:     return result;
> 1178:   }

I would remove this whole section since it's a functional change not covered by the issue description and unnecessary for your fix. 

It may also be wrong: loading shared objects without extension may be perfectly valid. The extension is just a convention.

src/hotspot/os/aix/os_aix.cpp line 1183:

> 1181:   // If the load fails,we try to reload by changing the extension to .a for .so files only.
> 1182:   // Shared object in .so format dont have braces, hence they get removed for archives with members.
> 1183:   if (result == nullptr) {

Suggestion:

  if (result == nullptr && pointer_to_dot != nullptr && strcmp(pointer_to_dot, old_extension) == 0) {

src/hotspot/os/aix/os_aix.cpp line 1184:

> 1182:   // Shared object in .so format dont have braces, hence they get removed for archives with members.
> 1183:   if (result == nullptr) {
> 1184:     assert(strlen(new_extension) < strlen(old_extension), "New extension length must be less than existing one");

Can be removed if you do the STATIC_ASSERT suggested above.

src/hotspot/os/aix/os_aix.cpp line 1186:

> 1184:     assert(strlen(new_extension) < strlen(old_extension), "New extension length must be less than existing one");
> 1185:     if (strcmp(pointer_to_dot, old_extension) == 0) {
> 1186:       sprintf(pointer_to_dot, "%s", new_extension);

Use os::snprintf, and limit output buffer by size of old extension.

-------------

PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1899612274
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1501757595
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1501754431
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1501754774
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1501757736
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1501757772
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1501757844