On Tue, 20 Feb 2024 09:27:15 GMT, Suchismith Roy <s...@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
