Rework `dlsym()`, `dlopen()`, `jl_dlsym()` and `jl_load_dynamic_library()` APIs by staticfloat · Pull Request #28888 · JuliaLang/julia

staticfloat · 2018-08-24T23:44:49Z

Remove jl_dlsym_e() to instead be rolled into jl_dlsym() with a throw_err parameter, similar to jl_load_dynamic_library_().
Fix jl_dlsym incorrectly assumes a NULL return is an error #28881 by having Libdl.dlsym() return nothing on missing symbol, rather than C_NULL.
Change dlopen() to mimic dlsym() so that a negative result returns nothing. While not necessary in the same way as in the dlsym() case (there are no valid NULL dlopen() results) the consistency is worth it.

Keno · 2018-08-25T01:21:05Z

src/dlload.c

+    /* Next, check for errors.  On Windows, a NULL pointer means the symbol
+     * was not found.  On everything else, we can have NULL symbols, so we check
+     * for non-NULL returns from dlerror().  */
+    const char * err = jl_dlerror();


Not a big deal, but we don't really need to do this work on windows unless we know there's a failure.

Yes, I originally had it split out but it was enough extra code that I liked it better this way. Optimizing for the programmer, not the program, I suppose.

Keno · 2018-08-25T01:33:52Z

src/dlload.c

 {
 #ifdef _OS_WINDOWS_
-    CHAR reason[256];
+    static CHAR reason[256];


This makes this no longer thread safe, which is probably fine if we only do it in the error case, but if we do it in the success case as well, I'm worried the error message will get overwritten by the success of a different thread.

I've fixed this by slapping __thread in front of it.

Unfortunately, we can't assume that that's universally available.

I was taking my inspiration from support/dirname.c. Can I do:

# if !defined(_COMPILER_MICROSOFT_) static __thread char reason[256]; # else static __declspec(thread) char reason[256]; # endif

If you really need to, you can use our own tls... There're already platform dependent members in there so it shouldn't be a huge problem even though it won't look as nice and will certainly needs cross references in the comments.

Because there seems to be some disagreement within the Julia codebase, I have taken the liberty of adding #define JL_THREAD_LOCAL to julia.h which emulates what is held within src/support/dirname.c. If this shouldn't be done (and we should instead use the more complex TLS implementation in src/threading.c), it's probably best to transition both jl_dlerror() and dirname() to use that new stuff instead.

Keno · 2018-08-25T01:36:04Z

Do you want to separate out the .c change from the julia changes to make that part eligible for backporting and use from BinaryProvider?

yuyichao · 2018-08-25T01:46:48Z

src/ccall.cpp

    };
 #define is_libjulia_func(name) _is_libjulia_func((uintptr_t)&(name), #name)

+    static auto ptls_getter = &jl_get_ptls_states;


Why the change? This completely defeated the purpose of the old code, which is clearly stated in the comment..............

It's because I have been dealing with the dynamic (runtime) linker too much, so when it said "can cause linker issues" I naively thought that meant at runtime when the symbol actually gets used. I didn't realize this caused issues at compile-time link. :P

You are welcome to change the comment in whichever way you want so that anyone that have read that comment won't change this code without testing the compiler configure combination again.....

staticfloat · 2018-08-26T22:55:13Z

With regards to splitting out the .c changes and the .jl changes; I don't really see how they can be changed; the .jl changes are dependent upon the .c changes, and the .c changes are not backwards-compatible.

That's okay though; because BP doesn't strictly need this; the libstdc++ version detection is future-proofing against a Julia version that doesn't have libgfortran linked into it, which doesn't exist yet.

staticfloat · 2018-08-27T00:11:05Z

ERROR: could not load symbol "SymRefreshModuleList":
The operation completed successfully.

^^ Due to a missing semicolon. Impressive.

staticfloat · 2018-08-27T15:40:10Z

Huzzah! We are passing all tests. If I could get a thumbs up or thumbs down on the API exposed here, then we can make the final decision.

I have purposefully not put in deprecation warnings for the *_e() methods in Julia, mostly because it would be very annoying for no real benefit at the moment. However it is something that should eventually be done, perhaps once we are closer to 2.0.

Keno · 2018-08-28T17:02:04Z

Ok by me. @yuyichao ?

yuyichao

LGTM. The ccall ptls getter lookup is correct but is just very confusing though....

yuyichao · 2018-08-28T17:53:27Z

src/ccall.cpp

+    // on some configurations (e.g. AArch64 + -Bsymbolic-functions), so we guard the
+    // `&jl_get_ptls_states` within this `#ifdef` guard, and use a more roundabout
+    // method involving `jl_dlsym()` on Linux platforms instead.
 #ifdef _OS_LINUX_


This actually makes the static useless. The performance isn't super critical here so if you want to just get rid of the static that's fine. Otherwise you can write it as.

static auto ptls_getter = [] { #ifdef _OS_LINUX_ jl_ptls_t (*p)(void); jl_dlsym(....); return p; #else return jl_get_ptls_states; #endif }();

Wow, that's available even all the way back to GCC 4.8.5. I'm amazed. And yes, you're right this completely defeats static, but I like this fancy lambda initialization. I'm gonna go with that since compiler support is good enough.

This is standard C++11 feature and we require C++11 so that shouldn't be very surprising......

yuyichao · 2018-08-28T17:55:38Z

src/dlload.c

 {
 #ifdef _OS_WINDOWS_
-    CHAR reason[256];
+    static JL_THREAD_LOCAL CHAR reason[256];


No change needed but note that the "more complex" tls implementation we have should actually be simpler and faster.

Simpler than just writing JL_THREAD_LOCAL? How would I use it?

Simpler in the code generated that is.

.

* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`. * Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.

This changes `dlopen()` to mimic `dlsym()` so that a negative result returns `nothing`. While not necessary in the same way as in the `dlsym()` case (there are no valid `NULL` `dlopen()` results) the consistency is worth it.

…c` again

staticfloat · 2018-08-28T23:19:42Z

When this goes green, I'm going to merge. Thanks for the shepherding, Keno and Yichao!

vtjnash · 2018-08-29T00:11:30Z

I'm not sure that I like the new API any better than the old, but I suppose that's not very helpful feedback.

staticfloat · 2018-08-29T04:25:11Z

Feel free to suggest a better API in a new PR. I’m going to sit with his one for now.

staticfloat · 2018-08-29T20:24:58Z

The behavioral change of returning nothing instead of just C_NULL should probably be documented somewhere, but NEWS.md is empty. Should this be added to that file?

KristofferC · 2018-08-29T21:28:33Z

Seems like the correct place to me.

staticfloat changed the title ~~Rework Libdl.dlsym() and jl_dlsym().~~ Rework Libdl.dlsym(), Libdl.dlopen(), jl_dlsym() and jl_load_dynamic_library() APIs Aug 24, 2018

staticfloat changed the title ~~Rework Libdl.dlsym(), Libdl.dlopen(), jl_dlsym() and jl_load_dynamic_library() APIs~~ Rework dlsym(), dlopen(), jl_dlsym() and jl_load_dynamic_library() APIs Aug 24, 2018

Keno reviewed Aug 25, 2018

View reviewed changes

yuyichao previously requested changes Aug 25, 2018

View reviewed changes

staticfloat force-pushed the sf/dlsym branch from 0fe3bde to 3322663 Compare August 26, 2018 22:48

staticfloat force-pushed the sf/dlsym branch 4 times, most recently from d517d3a to cac021c Compare August 26, 2018 23:43

staticfloat force-pushed the sf/dlsym branch 3 times, most recently from 119d60d to 29398e6 Compare August 27, 2018 06:49

yuyichao reviewed Aug 28, 2018

View reviewed changes

staticfloat added 7 commits August 28, 2018 15:30

Rework Libdl.dlsym() and jl_dlsym().

7080155

* Remove `jl_dlsym_e()` to instead be rolled into `jl_dlsym()` with a `throw_err` parameter, similar to `jl_load_dynamic_library()`. * Fix #28881 by having `Libdl.dlsym()` return `nothing` on missing symbol, rather than `C_NULL`.

Perform the same treatment to dlopen() for consistency's sake

2878358

This changes `dlopen()` to mimic `dlsym()` so that a negative result returns `nothing`. While not necessary in the same way as in the `dlsym()` case (there are no valid `NULL` `dlopen()` results) the consistency is worth it.

Make jl_dlerror() thread-safe

4aedf52

Fix botching of compile-time link bug

cb35103

Add JL_THREAD_LOCAL #define

44b40fc

Add dlclose(::Nothing) that does nothing

487d7da

Explain ifunc AArch64 woes a little more thoroughly.

aced658

staticfloat force-pushed the sf/dlsym branch from 29398e6 to 5921989 Compare August 28, 2018 22:31

Use lambda-initialization for ptls_getter to reap rewards of `stati…

5921989

…c` again

staticfloat merged commit fc27b48 into master Aug 29, 2018

staticfloat deleted the sf/dlsym branch August 29, 2018 04:25

staticfloat mentioned this pull request Aug 29, 2018

Fix Libdl changes #28953

Merged

vchuravy mentioned this pull request Sep 1, 2018

AddressSanitizer found buffer-overflow during jl_restore_system_image #28995

Closed

This was referenced Oct 10, 2018

ERROR: LoadError: LoadError: libstdc++ ABI version '3.4.0' too small; please build Julia with GCC >= 4.8.5, when Julia built with GCC 7.3 JuliaPackaging/BinaryProvider.jl#121

Closed

Fix issue #121 JuliaPackaging/BinaryProvider.jl#125

Merged

maleadt mentioned this pull request Nov 27, 2018

Don't restore globalUnique. #30166

Merged

Uh oh!

Conversation

staticfloat commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Keno commented Aug 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staticfloat commented Aug 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staticfloat commented Aug 27, 2018

Uh oh!

staticfloat commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keno commented Aug 28, 2018

Uh oh!

yuyichao left a comment

Choose a reason for hiding this comment

Uh oh!

yuyichao Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staticfloat commented Aug 28, 2018

Uh oh!

vtjnash commented Aug 29, 2018

Uh oh!

staticfloat commented Aug 29, 2018

Uh oh!

staticfloat commented Aug 29, 2018

Uh oh!

KristofferC commented Aug 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

staticfloat commented Aug 26, 2018 •

edited

Loading

staticfloat commented Aug 27, 2018 •

edited

Loading

yuyichao Aug 28, 2018 •

edited

Loading