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

add option to dlopen with RTLD_GLOBAL (fixes issue #2312) #2317

Closed
wants to merge 2 commits into from

Conversation

stevengj
Copy link
Member

In this patch, Julia's dlopen now has an optional boolean second argument a variant dlopen_global, which loads the shared library globally if possible (so that its symbols are available to any subsequently loaded shared library), needed to fix #2312.

Alternatively, instead of the optional second argument I could just rename that version to dlopen_global. Let me know what you prefer. Update: I renamed it this way below.

Works on Linux (and should work on other POSIX systems). It's not clear if Windows supports or needs this option; the flag currently does nothing on Windows.

@StefanKarpinski
Copy link
Sponsor Member

I'm gonna let @JeffBezanson sign off on this one, but this looks good to me. Nice addition.

@stevengj
Copy link
Member Author

I went ahead and renamed the new functionality to dlopen_global(libname) rather than dlopen(libname, true), as the former seems like it leads to more readable code.

@pao
Copy link
Member

pao commented Feb 16, 2013

Another option: dlopen(libname, :RTLD_GLOBAL)

@StefanKarpinski
Copy link
Sponsor Member

Actually preferred the second Boolean argument

@stevengj
Copy link
Member Author

@StefanKarpinski, this is from the person who wanted norm1, norm2, etcetera instead of norm(x,p) (#1875)? :-)

The reason I switched it was that when I looked at my own code, dlopen(libname, true) looked rather obscure: the meaning of the true argument is very nonobvious. Whereas dlopen_global(libname) at least gives a hint of the way in which it differs from a standard dlopen.

@StefanKarpinski
Copy link
Sponsor Member

Well, different norms are different functions. This does the same thing a slightly different way...

@stevengj
Copy link
Member Author

@StefanKarpinski, to nitpick, I would say that dlopen_global is a "different function" too. It doesn't merely differ in the algorithm, because the return value is inequivalent that of dlopen (the returned libraries behave differently in the face of inter-library dependencies). Of course, the functions are closely related, but so are norm1 and norm2.

But is this really the basis on which to decide such an issue, as opposed to e.g. code readability? (Or maintenance concerns, etcetera?)

@pao
Copy link
Member

pao commented Feb 16, 2013

dlopen() has seven currently defined flags according to the man page.

@nolta
Copy link
Member

nolta commented Feb 16, 2013

dlopen() has seven currently defined flags according to the man page.

On linux. POSIX only defines 4.

Pedantic achievement unlocked!

@stevengj
Copy link
Member Author

@pao, note that three of those 7 flags are Linux-only, and the remaining two are two pairs of opposites, so POSIX actually only gives two bits (two booleans) worth of choices. The other choice is RTLD_LAZY versus RTLD_NOW, and I'm not sure if there is a compelling reason to expose this in Julia.

(And then there is the question of Windows, which makes it especially iffy to think about exposing lots of dlopen flags unless they represent crucial functionality as in the case of RTLD_GLOBAL.)

@StefanKarpinski
Copy link
Sponsor Member

Pedantic achievement unlocked!

This is my favorite comment in a long time. Laughed out loud.

@stevengj
Copy link
Member Author

By the way, another reason to use a different symbol is that this way I can use isdefined(:dlopen_global) to check whether this function is available, and fall back to dlopen if not. That way, I can still run under Julia 0.1 (albeit with reduced functionality).

(Or is there a similarly easy way to check whether a 2-argument version of a function is available?)

@StefanKarpinski
Copy link
Sponsor Member

method_exists(dlopen,(String,Bool)) would work for that.

@stevengj
Copy link
Member Author

Well, I've said my piece. When it comes down to it, I don't really care that much what the interface is, as long as the functionality is there. Say the word and I will revert the patch to dlopen(String,Bool) or whatever there is consensus for (or as our glorious leader @JeffBezanson commands).

@StefanKarpinski
Copy link
Sponsor Member

Jeff's call

@pao
Copy link
Member

pao commented Feb 16, 2013

Pedantic achievement unlocked!

Haha, fair enough. I agree that most of those are probably useless to us, or are indeed Linux-specific, but something about dlopen_global definitely makes me pause.

@JeffBezanson
Copy link
Sponsor Member

Both the bool argument and dlopen_global feel tacked on, but the second one is at least easier to read. It's not great that needing this flag means you have to use dlopen/dlsym instead of the usual ccall syntax. I wonder if there would be a major downside to setting RTLD_GLOBAL for everything?

@StefanKarpinski
Copy link
Sponsor Member

Another option (get it) is to actually use integer flags. Easier to read but still just an extra argument.

@JeffBezanson
Copy link
Sponsor Member

There is something to be said for using dlopen(l, RTLD_GLOBAL|...); if you already know about this stuff you can understand it immediately.

@stevengj
Copy link
Member Author

@JeffBezanson, the downside to using RTLD_GLOBAL for everything is the potential for symbol collisions, I think. Also, note that I have to use dlopen anyway, even without this issue, because of issue #2316.

Regarding dlopen(libname, RTLD_GLOBAL|...), what flags would you want to export besides RTLD_GLOBAL?

@stevengj
Copy link
Member Author

Just rebased the patch. Any additional thoughts?

@carlobaldassi
Copy link
Member

Small note: opening with RTLD_GLOBAL would also help with the Clp.jl package (we currently apply a pacth to the Makefile in order to work around the issue, but this would allow using system binaries).

@stevengj
Copy link
Member Author

@JeffBezanson, can I go ahead and merge or do you want a different spelling?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a way to use shared libraries with RTLD_GLOBAL flag
6 participants