Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8320890: [AIX] Find a better way to mimic dl handle equality #16920

Closed
wants to merge 14 commits into from

Conversation

JoKern65
Copy link
Contributor

@JoKern65 JoKern65 commented Dec 1, 2023

On AIX, repeated calls to dlopen referring to the same shared library may result in different, unique dl handles to be returned from libc. In that it differs from typical libc implementations that cache dl handles.

This causes problems in the JVM with code that assumes equality of handles. One such problem is in the JVMTI agent handler. That problem was fixed with a local fix to said handler (JDK-8315706). However, this fix causes follow-up problems since it assumes that the file name passed to os::dll_load() is the file that has been opened. It prevents efficient, os_aix.cpp-local workarounds for other AIX issues like the .so/.a duality. See JDK-8320005. As such, it is a hack that causes other, more uglier hacks to follow (see discussion of #16604).

We propose a different, cleaner way of handling this:

  • Handle this entirely inside the AIX versions of os::dll_load and os::dll_unload.
  • Cache dl handles; repeated opening of a library should return the cached handle.
  • Increase handle-local ref counter on open, Decrease it on close
  • Make sure calls to os::dll_load are matched to os::dll_unload (See JDK-8320830).

This way we mimic dl handle equality as it is implemented on other platforms, and this works for all callers of os::dll_load.


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-8320890: [AIX] Find a better way to mimic dl handle equality (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16920

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 1, 2023

👋 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 openjdk bot added the rfr Pull request is ready for review label Dec 1, 2023
@openjdk
Copy link

openjdk bot commented Dec 1, 2023

@JoKern65 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 openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Dec 1, 2023
@mlbridge
Copy link

mlbridge bot commented Dec 1, 2023

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, this is how I have pictured a good solution. Very nice.

A number of remarks, but nothing fundamental.

};
constexpr int max_handletable = 1024;
static int g_handletable_used = 0;
static struct handletableentry g_handletable[max_handletable] = {{0,0,0,0}};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nits:

  • we usually write the * behind type, not before var name
  • {{0,0}} -> insert spaces

// file with filename does not exist
result = ::dlopen(filename, dflags);
if (result != nullptr) {
assert(false, "dll_load: Could not stat() file %s, but dlopen() worked; Have to improve stat()", filename);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use assert(result != nullptr) and remove condition

::strncpy(ebuf, "dll_load: empty filename specified", ebuflen - 1);
if (ebuf != nullptr && ebuflen > 0) {
::strncpy(ebuf, "dll_load: empty filename specified", ebuflen - 1);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any cases where we don't hand in the error buffer? If so, I would just assert ebuf and ebuflen. No need for this kind of flexibility.

@@ -3033,29 +3049,134 @@ void os::jfr_report_memory_info() {}

// Simulate the library search algorithm of dlopen() (in os::dll_load)
int os::Aix::stat64x_via_LIBPATH(const char* path, struct stat64x* stat) {
Copy link
Member

@tstuefe tstuefe Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • no need to export this, make it filescope static
  • please return bool, with false = error
  • please rename it to something like "search_file_in_LIBPATH"

// if exist, strip off trailing (shr_64.o) or similar
int idx = strlen(path2) - 1;
if (path2[idx] == ')') {
while (path2[idx] != '(' && idx > 0) idx--;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? Why not strrchr()?

return result;
}

int os::Aix::dlclose(void* lib) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we call lib something better, maybe "handle"?

}
// refcount == 0, so we have to ::dlclose() the lib
// and delete the entry from the array.
res = ::dlclose(lib);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle dlclose error. We expect it to work; if it doesn't, it indicates that something is wrong with the handle logic, e.g. an invalid or already closed handle had been handed in. So, assert.

// specific AIX versions for ::dlopen() and ::dlclose(), which handles the struct g_handletable
// filled by os::dll_load(). This way we mimic dl handle equality for a library
// opened a second time, as it is implemented on other platforms.
void* os::Aix::dlopen(const char* filename, int Flags) {
Copy link
Member

@tstuefe tstuefe Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not need to be exported, nor does os::AIX::dlclose. Make file scope static. See my remarks in os_posix.cpp.

// filled by os::dll_load(). This way we mimic dl handle equality for a library
// opened a second time, as it is implemented on other platforms.
static void* dlopen(const char* filename, int Flags);
static int dlclose(void* lib);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove; should not be exported.

@@ -731,7 +732,7 @@ void os::dll_unload(void *lib) {
if (l_path == nullptr) {
l_path = "<not available>";
}
int res = ::dlclose(lib);
int res = AIX_ONLY(os::Aix)::dlclose(lib);
Copy link
Member

@tstuefe tstuefe Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets do this cleaner, and in the style of hotspot coding elsewhere:

  • introduce a new function "os::pd_dll_unload(handle, errorbuf, errbuflen)". Add it to os.hpp, but somewhere non-public. The implementations will live in os_aix.cpp, os_bsd.cpp and os_linux.cpp.
  • make os::Aix::dlclose -> os::pd_dll_unload; the only difference is that you should fill in error buffer with either ::dlerror or, if you have errors in handle table, a text describing that error
  • on all other posix platforms (os_bsd.cpp + os_linux.cpp), implement a minimal version of os::pd_dll_unload() that calls ::dlunload, and on error calls ::dlerror and copies the string into errbuf
  • Here, call os::pd_dll_unload instead of ::dlclose/os::aix::dlclose
  • change the JFR code below to not use ::dlerror but the string returned from the buffer

@suchismith1993
Copy link
Contributor

@tstuefe Sorry to tag you. Can you review the code. Once this code goes in I can push in my changes.
We are targeting the fix for January.

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this libpath parsing code copied from the R3 kernel? If yes, pls make sure there are no licensing issues.

};
constexpr int max_handletable = 1024;
static int g_handletable_used = 0;
static struct handletableentry g_handletable[max_handletable] = {{0, 0, 0, 0}};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move all that new and clearly delineated dlopen stuff into an own file, e.g. dlopen_aix.cpp or porting_aix.cpp (in porting_aix.cpp, we already have wrappers for other functions). os_aix.cpp is already massive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the static variable declarations and the functions Aix_dlopen(), search_file_in_LIBPATH(), rtv_linkedin_libpath() and os::pd_dll_unload() to porting_aix.cpp. This links, but in my opinion os::pd_dll_unload() should reside in os_aix.cpp, because it is member of the os class. But there it will not compile anymore if the static variables are moved away.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, what I meant was to provide a "libc-like" equivalent for dlopen, similar to what we do with dladdr (see

int dladdr(void* addr, Dl_info* info) {
).

But never mind; I am also fine with moving os::pd_dlopen into a different cpp file, e.g. "dlopen_aix.cpp". Just move it out of os_aix.cpp, since that is already massive and you add >300 lines of more code and more dependencies.

static const char* libpath = 0;

if (libpath)
return libpath;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -1108,6 +1125,282 @@ bool os::dll_address_to_library_name(address addr, char* buf,
return true;
}

// get the library search path burned in to the executable file during linking
// If the libpath cannot be retrieved return an empty path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new. Is this complexity needed, if yes, why? Don't see a comment, may have missed it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why are we parsing xcoff32 headers in there? AIX OpenJDK will always be 64-bit. So, you can replace the whole xcoff32 section with assert( f_magic == U802TOCMAGIC, ..). The function becomes a lot simpler then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a leak in my previous implementation. It is more or less academical, but this solution is the complete one. I would prefer this complete solution even it is complex, because if dlopen follows a slightly different algorithm in resolving the library we surely get into trouble.
If we omit the xcoff32 we have to ensure that no xcoff32 executable file comes into play.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we omit the xcoff32 we have to ensure that no xcoff32 executable file comes into play.

xcoff32 is for 32-bit binaries. The AIX port only exists for 64-bit, and there will never be a 32-bit AIX port, so there is no reason for handling 32-bit xcoff headers.

if (libpath)
return libpath;

char pgmpath[32+1];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will overflow if pid_t is 64bit. Give it a larger size; after all, you are giving buffer 4K above, so you are not overly concerned with saving stack space.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adopted. use buffer instead of pgmpath

// get the library search path burned in to the executable file during linking
// If the libpath cannot be retrieved return an empty path
static const char* rtv_linkedin_libpath() {
static char buffer[4096];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This coding has some issues:

  • a generic char buffer is not a good idea. Forces you to do casts all over the place, and introduces alignment issues with unaligned char buffer. Which I assume is the reason for all the separate memcpy-into-structures below. I would just read into the structures directly.
  • you need to check the return codes for fread to make sure you read the number of bytes expected, lest you work with uninitialized memory and maybe to handle sporadic EINTR.
  • I don't get all the separate "SZ" macros. They must be equal to sizeof(structure), right, otherwise you get buffer overruns or work with uninitialized memory?

Proposal: add a local wrapper function like this:

template <class T>
static bool my_checked_fread(FILE* f, T* out) {
  // read sizeof(T) from f. 
  // Check return code. 
  // Return bool if sizeof(T) bytes were read.
  e.g. in a very trivial form:
  int bytesread = fread(out, sizeof(T), 1, f);
  return bytesread == sizeof(T);
}

and use it in your code like this:

struct xcoff64 the_xcoff64;
struct scn64 the_scn64;
struct ldr64 the_ldr64;

if (!my_checked_fread(f, &the_xcoff64)) {
   assert?
}
...

if (!my_checked_fread(f, &the_ldr64) { 
  .. handle error  
}

struct _S_(scnhdr) scn64;
struct _S_(ldhdr) ldr64;
memcpy((char*)&xcoff64, buffer, FILHSZ_64 + _AOUTHSZ_EXEC_64);
int ldroffset = FILHSZ_64 + xcoff64.filehdr.f_opthdr + (xcoff64.aouthdr.o_snloader -1)*SCNHSZ_64;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the -1? I assume thats the section number? is it 1 based? how odd..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the section numbers are 1 based. e.g. Beginning of section 4 has an offset of 3 section sizes.

// If the libpath cannot be retrieved return an empty path
static const char* rtv_linkedin_libpath() {
static char buffer[4096];
static const char* libpath = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your intent is to return an empty buffer if there is no contained libpath, I would just:

static const char* libpath = "";

then you can always just return libpath.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But looking at the using code, returning NULL in case there is no contained libpath would be actually easier, see below.

fread(buffer, 1, 4096, f);
}
else
buffer[0] = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, due to complete rewriting. s.o.

fseek (f, scn64.s_scnptr, SEEK_SET);
fread(buffer, 1, LDHDRSZ_64, f);
memcpy((char*)&ldr64, buffer, LDHDRSZ_64);
fseek (f, scn64.s_scnptr + ldr64.l_impoff, SEEK_SET);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please use consistent spacing according to hotspot rules. here, remove space.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the space fseek ( ?
Done.

}

stringStream Libpath;
if (env == nullptr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal for shorter version not needing string assembly:

const char* paths [2] = { env, rtv_linkedin_libpath() }:
for (int i = 0; i < 2; i ++) {
  const char* this_libpath = paths[i];
  if (this_libpath == nullptr) {
    continue;
  }
    ... do the token thing...
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I did not clearly understand how this should work. The mystery must be in ... do the token thing ...

@JoKern65
Copy link
Contributor Author

The libpath parsing code is from me, so no license problems.

@openjdk openjdk bot removed the rfr Pull request is ready for review label Dec 15, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 15, 2023
Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, this is good.

Small nits remain.

struct xcoffhdr the_xcoff;
struct scnhdr the_scn;
struct ldhdr the_ldr;
size_t sz = FILHSZ + _AOUTHSZ_EXEC;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rename to xcoffsz, and make constexpr: constexpr size_t xcoffsz = ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can you please add

STATIC_ASSERT(sizeof(the_xcoff) == xcoffsz);
STATIC_ASSERT(sizeof(the_scn) == SCNHSZ);
STATIC_ASSERT(sizeof(the_ldr) == LDHDRSZ);

const char* env = getenv("LIBPATH");
if (env == nullptr) {
// no LIBPATH, try with LD_LIBRARY_PATH
env = getenv("LD_LIBRARY_PATH");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is LD_LIBRARY_PATH a thing on AIX? I thought it is only used on non-AIX.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is, It's the fallback if LIBPATH is not defined

Copy link
Member

@tstuefe tstuefe Dec 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case there may be errors in other places, since so far we assumed its either one or the other, but not both. Example:

https://github.com/openjdk/jdk/blob/a247d0c74bea50f11d24fb5f3576947c6901e567/src/java.base/unix/native/libjli/java_md.c#L43C1-L47

Maybe you need to take a look here, in case LD_LIBRARYPATH needs to be handled in addition to LIBPATH?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's one or the other. If LIBPATH envvar exists (even empty string), LD_LIBRARY_PATH is ignored. So, no problems.

// LIBPATH or LD_LIBRARY_PATH given with content -> try first with
// LIBPATH or LD_LIBRARY_PATH and second with burned in libpath.
// No check against current working directory
Libpath.print("%s:%s", env, rtv_linkedin_libpath());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure libpath env var has precedence over the baked-in libpath?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was the outcome of my experiments, although the IBM docu says the oposite:
"Specifies that the library path used at process exec time should be prepended to any library path specified in the load call (either as an argument or environment variable). It is recommended that this flag be specified in all calls to the load subroutine."

My experiment showed: LIBPATH=libpath; baked-in-libpath=baked-in-libpath;
mylib.so is in both paths. After dlopen(mylib.so) a map call shows the library was loaded from libpath.
Then I remove the LIBPATH envvar and repeat. Now after dlopen(mylib.so) a map call shows the library was loaded from baked-in-libpath.
So the LIBPATH envvar has precedence over the baked-in-libpath.

// try to find handle in array, which means library was loaded by os::dll_load() call
for (i = 0; i < g_handletable_used; i++) {
if (g_handletable[i].handle == libhandle) {
// handle found, decrease refcount
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert(refcount > 0, "Sanity"))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

ebuf[ebuflen - 1] = '\0';
}

pthread_mutex_lock(&g_handletable_mutex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can make your life a lot easier by defining an RAII object at the start of the file:

struct TableLocker {
  TableLocker() { pthread_mutex_lock(&g_handletable_mutex); }
  ~TableLocker() { pthread_mutex_unlock(&g_handletable_mutex); }
};

and just place this at the beginning of your two functions

TableLocker lock:
...

no need to manually unlock then, with the danger of missing a return.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you. This was one of the things I thought about, but was not sure, because I did not fully understood the MutexLocker class and the difference between Monitor and Mutex.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hindsight, pthread mutex is the better choice anyway: it avoids dependencies to the VM lifecycle (VM mutexes are only available after VM initialization, so we could not call dlopen() before that).

// is sufficient to remove the entry from the array, otherwise we move the last
// entry of the array to the place of the entry we want to remove and overwrite it
if (i < g_handletable_used) {
g_handletable[i] = g_handletable[g_handletable_used];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be super careful, I would zero out at least the handle of the moved item like this:
g_handletable[g_handletable_used].handle = nullptr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -27,6 +27,7 @@
// with C++ compiler before referencing the function alloca()
#pragma alloca


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove whitespace change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -189,6 +190,7 @@ int os::Aix::_extshm = -1;
////////////////////////////////////////////////////////////////////////////////
// local variables


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove whitespace change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1108,6 +1110,7 @@ bool os::dll_address_to_library_name(address addr, char* buf,
return true;
}


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove whitespace change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done.

Releasing the mutex before asserting is not necessary; we don't pull the handle table lock as part of error reporting.

@openjdk
Copy link

openjdk bot commented Dec 18, 2023

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

8320890: [AIX] Find a better way to mimic dl handle equality

Reviewed-by: stuefe, mdoerr

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

  • e5aed6b: 8323276: StressDirListings.java fails on AIX
  • b922f8d: 8319793: C2 compilation fails with "Bad graph detected in build_loop_late" after JDK-8279888
  • 35e9662: 8314515: java/util/concurrent/SynchronousQueue/Fairness.java failed with "Error: fair=false i=8 j=0"
  • cb1d25f: 8323330: [BACKOUT] JDK-8276809: java/awt/font/JNICheck/FreeTypeScalerJNICheck.java shows JNI warning on Windows
  • 2b7fc05: 8264102: JTable Keyboards Navigation differs with Test Instructions.
  • af942a6: 8323188: JFR: Needless RESOURCE_ARRAY when sending EventOSInformation
  • 26de9e2: 8321616: Retire binary test vectors in test/jdk/java/util/zip/ZipFile
  • b530c02: 8317804: com/sun/jdi/JdwpAllowTest.java fails on Alpine 3.17 / 3.18
  • e70cb4e: 8322565: (zipfs) Files.setPosixPermissions should preserve 'external file attributes' bits
  • d89602a: 8322982: CTW fails to build after 8308753
  • ... and 396 more: https://git.openjdk.org/jdk/compare/ecd335d8f42757d332f217e220e1a9db8c48c8d6...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 (@tstuefe, @TheRealMDoerr) 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 openjdk bot added the ready Pull request is ready to be integrated label Dec 18, 2023
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like getting rid of #ifdef AIX in shared code. The change is not simple, but looks basically good to me. I'll take a closer look when I find more time. I have some coding style requests. Please also see https://wiki.openjdk.org/display/HotSpot/StyleGuide (especially section Whitespace).

@@ -1108,6 +1110,7 @@ bool os::dll_address_to_library_name(address addr, char* buf,
return true;
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

libpath = buffer;

return libpath;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty line could get removed.

// But if FilePath does not start with / or . we have to prepend it with ./
if (strchr(path2, '/')) {
stringStream combined;
if (*path2 == '/' || *path2 == '.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually use { and } unless for extremely simple substatements on the same line

@@ -1063,6 +1063,9 @@ class os: AllStatic {
char pathSep);
static bool set_boot_path(char fileSep, char pathSep);

static bool pd_dll_unload(void* libhandle, char* ebuf, int ebuflen);


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove empty lines.

@suchismith1993
Copy link
Contributor

Hi @JoKern65 Is this good to integrate now ?

@JoKern65
Copy link
Contributor Author

Hi @suchismith1993, I'm waiting for a second review. Complex hotspot changes should be reviewed twice.

@tstuefe
Copy link
Member

tstuefe commented Dec 19, 2023

@tstuefe Sorry to tag you. Can you review the code. Once this code goes in I can push in my changes.
We are targeting the fix for January.

Hi @JoKern65 Is this good to integrate now ?

@suchismith1993 Please don't put pressure on patch authors and developers. There is zero reason why this patch should be rushed.

Hi @suchismith1993, I'm waiting for a second review. Complex hotspot changes should be reviewed twice.

Not only that, hotspot changes need to be reviewed by at least two reviewers. That is not optional. See OpenJDK bylaws.

@tstuefe
Copy link
Member

tstuefe commented Dec 19, 2023

@suchismith1993

Once this code goes in I can push in my changes. We are targeting the fix for January.

If you talk about #16604, no, you cannot push that even if Joachim finishes his work.

Your patch has not even a single review, is quite controversial, and none of the issues the reviewers have mentioned are addressed. This needs a lot more discussion time.

@suchismith1993
Copy link
Contributor

@suchismith1993

Once this code goes in I can push in my changes. We are targeting the fix for January.

If you talk about #16604, no, you cannot push that even if Joachim finishes his work.

Your patch has not even a single review, is quite controversial, and none of the issues the reviewers have mentioned are addressed. This needs a lot more discussion time.

I have the patch ready based on the changes in this patch, as I take the diff and apply. But I cannot push since it will end up adding the entire file.

@suchismith1993
Copy link
Contributor

suchismith1993 commented Dec 19, 2023

@tstuefe Sorry to tag you. Can you review the code. Once this code goes in I can push in my changes.
We are targeting the fix for January.

Hi @JoKern65 Is this good to integrate now ?

@suchismith1993 Please don't put pressure on patch authors and developers. There is zero reason why this patch should be rushed.

Hi @suchismith1993, I'm waiting for a second review. Complex hotspot changes should be reviewed twice.

Not only that, hotspot changes need to be reviewed by at least two reviewers. That is not optional. See OpenJDK bylaws.

Sorry about that. The fix was critical for the adoptium builds and hence was looking to fix this soon.

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.