LibELF: Implement dlclose similar to how it is done in glibc #24264
Conversation
github-actions
bot
added
the
👀 pr-needs-review
| @@ -45,7 +45,7 @@ namespace { | |||
| ByteString s_main_program_path; | |||
|
|
|||
| // The order of objects here corresponds to the "load order" from POSIX specification. | |||
| OrderedHashMap<ByteString, NonnullRefPtr<ELF::DynamicObject>> s_global_objects; | |||
| OrderedHashMap<ByteString, NonnullOwnPtr<ELF::DynamicObject>> s_global_objects; | |||
There was a problem hiding this comment.
Not sure bout this one. If we need fancy ref-coutning for Dynamic Object, the proper way would be to implement that custom ref-counting on the object directly, rather than inheriting from RefCounted. RefPtr doesn't require you to use RefCounted, simply that the object has the required ref/unref calls.
There was a problem hiding this comment.
I found it easier to reason about the DynamicObjects' lifetimes when there's a single owner and two separate refcounts that aren't obligatory.
Oh, and there is exactly one place where the objects are created, and exactly one place where they get destroyed.
There was a problem hiding this comment.
Looking into the next commit a bit, I'm not sure what the difference between your proposed whacky ownership model and the "normal" RefPtr model is; dlopen_ref and dlopen_unref are just ref and unref, your manual unref call is exactly equivalent to setting the pointer to null. your manual ref calls can stay as they are.
The index will be used to efficiently record unload dependencies of shared object.
We need a much more convoluted way to count references to DynamicObject than RefPtr provides to properly support dlclose.
While dlclose is not used often in the wild (and we certainly don't have references to it in Serenity) and is not required by POSIX to do anything, I would rather copy glibc's behavior instead of leaving musl-like no-op behavior. Implementing it properly also motivates a future in-house __cxa_thread_atexit.
| return position.value(); | ||
| } else { |
| @@ -45,7 +45,7 @@ namespace { | |||
| ByteString s_main_program_path; | |||
|
|
|||
There was a problem hiding this comment.
I still don't understand why we need this.
Most libcs I know of don't go through all this to support something basically no one relies on.
| @@ -45,7 +45,7 @@ namespace { | |||
| ByteString s_main_program_path; | |||
|
|
|||
| // The order of objects here corresponds to the "load order" from POSIX specification. | |||
| OrderedHashMap<ByteString, NonnullRefPtr<ELF::DynamicObject>> s_global_objects; | |||
| OrderedHashMap<ByteString, NonnullOwnPtr<ELF::DynamicObject>> s_global_objects; | |||
There was a problem hiding this comment.
Looking into the next commit a bit, I'm not sure what the difference between your proposed whacky ownership model and the "normal" RefPtr model is; dlopen_ref and dlopen_unref are just ref and unref, your manual unref call is exactly equivalent to setting the pointer to null. your manual ref calls can stay as they are.
| @@ -457,6 +457,31 @@ static ErrorOr<void, DlErrorMessage> link_main_library(int flags, DependencyOrde | |||
| return {}; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
While dlclose is not used often in the wild (and we certainly don't have references to it in Serenity) and is not required by POSIX to do anything, I would rather copy glibc's behavior instead of leaving musl-like no-op behavior.
Why tho?
I'm sorry, but I read that as:
While dlclose doing anything is literally useless, I want to make it do something.
|
(Off: and that was me being "excessively" emotional with a single "no, no, no" in the review of 21101 :caret:) Well, I wrote the code, it hasn't felt like something horrible, I thought "fine, let's pr it, I don't care if it gets accepted or not". I think a single objection is enough though to not merge this. |
github-actions
bot
removed
the
👀 pr-needs-review
|
I'm not really objecting to it, rather asking you what its purpose is -- is there something you want to do that needs this? some code that does something fun when this behaviour is supported? etc etc. |
|
I was just convinced that every self-respecting libc should implement dlclose not as a no-op (but that's clearly not the case, see musl). There will indeed be no use for this code currently or in the near future. (And if there ever will be a need, PR is not going anywhere) |
This resolves multiple fixmes in a way original authors intended them to be resolved. I'm still not convinced we need "proper" dlclose but noone seemed to have any strong opinions about it when I asked on Discord.
CC @supercomputer7, you might want to say something about the design of dlclose promise.
PS: Most of the diff is tests, the actual change is about +120 net added loc.