-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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-8288003: log events for os::dll_unload #9101
Conversation
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally okay but I'd like to minimise the conditional code.
Thanks.
src/hotspot/os/posix/os_posix.cpp
Outdated
#if defined(LINUX) | ||
struct link_map *lmap; | ||
int res_dli = ::dlinfo(lib, RTLD_DI_LINKMAP, &lmap); | ||
const char* l_path = "<none>"; | ||
if (res_dli == 0) { | ||
l_path = lmap->l_name; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we hide this in os::Linux::dll_path
so that the code here would simply be:
const char* l_path = LINUX_ONLY(os::Linux::dll_path(lib))
NOT_LINUX("<not available>");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi David, thanks for the advice, done.
I also added some more quoting and added the dlerror information to the error report (see os_posix.cpp).
src/hotspot/os/posix/os_posix.cpp
Outdated
int res = ::dlclose(lib); | ||
|
||
if (res == 0) { | ||
Events::log_dll_message(NULL, "Unloaded shared library %s [" INTPTR_FORMAT "]", l_path, p2i(lib)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the %s be quoted here too? The output will look a bit odd:
Unloaded shared library [0xbaadcafebaadcafe]
vs.
Unloaded shared library "" [0xbaadcafebaadcafe]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - thanks for incorporating my requests.
One minor item below.
Thanks
src/hotspot/os/linux/os_linux.cpp
Outdated
const char* os::Linux::dll_path(void* lib) { | ||
struct link_map *lmap; | ||
int res_dli = ::dlinfo(lib, RTLD_DI_LINKMAP, &lmap); | ||
const char* l_path = "<none>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "not available" to match the non-Linux case?
@MBaesken 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:
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 38 new commits pushed to the
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. ➡️ To integrate this PR with the above commit message to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to add null check to dll_unload() because it might be different behavior between Linux and Windows.
GetModuleFileName() on Windows will return the path of executable if NULL is passed to hModule.
https://docs.microsoft.com/ja-JP/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulefilenamea
On the other hand, dlinfo() does not mention if NULL is passed in library handle.
https://man7.org/linux/man-pages/man3/dlinfo.3.html
I think dll_unload() is not expexted to unload executable...
Hello, I added a NULL check to os::Linux::dll_path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why didn't you add NULL check to os::dll_unload()
both os_windows and os_posix?
If NULL is passed to os::dll_unload()
at os_windows.cpp, it would be passed to GetModuleFileName()
, then we would get executable path.
I think we can add assert
to the start of both dll_unload()
because NULL shouldn't be passed to them.
::FreeLibrary((HMODULE)lib); | ||
char name[MAX_PATH]; | ||
if (::GetModuleFileName((HMODULE)lib, name, sizeof(name)) == 0) { | ||
name[0] = '\0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we set <not available>
to name
like a change for linux code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is most likely better to have same output in failure case on Windows and other platforms.
src/hotspot/os/linux/os_linux.cpp
Outdated
@@ -1786,6 +1786,18 @@ void * os::Linux::dll_load_in_vmthread(const char *filename, char *ebuf, | |||
return result; | |||
} | |||
|
|||
const char* os::Linux::dll_path(void* lib) { | |||
struct link_map *lmap; | |||
const char* l_path = "<not available>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I prefer to return NULL if dlinfo
is failed.
IIUC the caller expects return dll path of lib
- <not available>
is not the path.
I think it is better that alternative string would be set by the caller (os::dll_unload()
) like a change of os_windows. But I do not stick my opinion. I'm ok if other reviewer approve your change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted dll_path, it returns now NULL in case of failure.
I would expect NULL checks to be present in the calling code somewhere; at most an assertion for NULL should be present in this lowest-level code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Matthias,
this looks okay and is useful.
I worried a bit about the thread safety of ::dlerror(), since it is specified as not threadsafe by Posix: "The dlerror() function need not be reentrant. A function that is not required to be reentrant is not required to be thread-safe." .
However, the glibc variant (https://sources.debian.org/src/glibc/2.28-10/dlfcn/dlerror.c) seems to be thread-safe, using POSIX TLS to store the resulting error string.
But interestingly enough, the string itself only lives as long as ::dlerror() is not called again. A second call to dlerror() will free() the string. So, one should use the string right away, for storing it should be strduped.
I did not look if dlerror is threadsafe on Muslc.
src/hotspot/os/linux/os_linux.cpp
Outdated
const char* os::Linux::dll_path(void* lib) { | ||
struct link_map *lmap; | ||
const char* l_path = NULL; | ||
if (lib != NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still say this can just be an assert - it is checked in the callers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I introduced an assert in dll_path.
The usage here is no different to existing usages. |
Sure, but they could have been existing errors :-) For proof that these thoughts are not completely unfounded see e.g. https://gitlab.gnome.org/GNOME/glib/-/issues/399, in that case pertaining to the now defunct eglibc. But older versions of glibc were not threadsafe either. |
Sure, but my point is that this fix is not introducing anything new. The potential for a broken dlerror has been discussed in the past. I don't think it is something we need to try to jump through hoops to anticipate. It is rare enough we will get an error, let alone concurrent ones. |
Of course, I agree. Thats why I approved the patch. |
/integrate |
Going to push as commit c2ccf4c.
Your commit was automatically rebased without conflicts. |
Currently we only log events for os::dll_load, but not for os::dll_unload, this patch adds it. On some platforms (Linux/Windows) we can use OS APIs (e.g. dlinfo on Linux) to log the path of the unloaded shared lib.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9101/head:pull/9101
$ git checkout pull/9101
Update a local copy of the PR:
$ git checkout pull/9101
$ git pull https://git.openjdk.org/jdk pull/9101/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9101
View PR using the GUI difftool:
$ git pr show -t 9101
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9101.diff