Lookup `ccall` symbols in internal libraries first #49010

topolarity · 2023-03-15T13:39:10Z

~~This change lays the necessary groundwork to support performing versioned symbol lookups using dlvsym.~~

~~This is a pre-requisite for symbol-versioning Julia's internal libraries, and the new code paths here won't actually be exercised until a follow-up is landed to turn on symbol-versioning.~~

This is similar to what we do on Windows, where handles to the exact library are required anyway. This is enough to make sure that our dlsym lookups are directed to the correct libjulia, even when loading a Julia run-time within Julia.

staticfloat

All of the if statements everywhere make me think perhaps we should just have jl_dlsym()take a const char * version argument and use version == NULL as the typical jl_dlsym() case, and version != NULL as the jl_dlvsym() case. Taking a brief look at the glibc code, I don't think it's legal to pass version == NULL, so we should be okay to use that as a sentinel value for the non-versioned case.

That should clean up a lot of the usage here. If we want to avoid ABI breakage, we could just renamed jl_dlsym to jl_dlvsym add the version parameter, then add a new jl_dlsym that just calls jl_dlvsym with version set to NULL.

Then in codegen and everywhere else, we only emit calls to jl_dlvsym with version set to NULL in all non-versioned cases.

@topolarity @vtjnash thoughts?

topolarity · 2023-03-15T21:51:14Z

Sounds like a decent plan to me!

I'll make that change tomorrow unless any objections are raised before then.

Pangoraw · 2023-03-16T12:14:06Z

Would it make sense to expose the newly created jl_dlvsym as Julia Libdl.dlvsym()/cvcall or is this for internal use only ? I had the need for something similar recently (to ccall symbols from a versionned shared library).

topolarity · 2023-03-16T15:11:13Z

Heads up that this change is now ABI-breaking for the (internal) symbols jl_load_and_lookup and jl_lazy_load_and_lookup. Let me know if that's a problem.

Public ABI has new symbol jl_dlvsym and is otherwise unaffected.

Would it make sense to expose the newly created jl_dlvsym as Julia Libdl.dlvsym()/cvcall or is this for internal use only ? I had the need for something similar recently (to ccall symbols from a versionned shared library).

Sounds like a good idea to me, but I'd like to keep this PR minimal for now - Could you file an issue for your use case? We can discuss the details about how to expose versioned ccalls there.

staticfloat

This looks good to me, but I'd like to get @vtjnash's input

staticfloat · 2023-03-30T17:13:39Z

I spoke about this with Jameson, and he is curious to see if we can avoid surfacing dlsymv() at all, and simply require that all ccall() uses provide a library handle instead. There are a few cases where this is not properly being done (here's a prime example:

julia/base/stat.jl

Lines 63 to 74 in ceafd6c

    
           ccall(:jl_stat_dev,     UInt32,  (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_ino,     UInt32,  (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_mode,    UInt32,  (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_nlink,   UInt32,  (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_uid,     UInt32,  (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_gid,     UInt32,  (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_rdev,    UInt32,  (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_size,    UInt64,  (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_blksize, UInt64,  (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_blocks,  UInt64,  (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_mtime,   Float64, (Ptr{UInt8},), buf), 
        
           ccall(:jl_stat_ctime,   Float64, (Ptr{UInt8},), buf),

) but if we quash those, then we might be able to rely on symbol versioning to take care of matching the correct symbols for the dynamic linker, and then rely on providing the correct library handle for when we're doing ccall() or dlsym() lookups in Julia.

topolarity · 2023-03-31T13:26:58Z

and then rely on providing the correct library handle for when we're doing ccall() or dlsym() lookups in Julia.

It seems more breaking to me not to correctly support resolving Julia-exported symbols in the "local" runtime without a handle.

I'd prefer expanding the existing code where we intercept the symbol resolution to support this, which is exactly where #49012 currently adds a version-based lookup. I think this in principle would work with just dlsym and the right handle(s), but we'd want it to check for the non-"i"-prefixed version of the symbol as well.

Unfortunately, when I tested yesterday that doesn't work, and I don't understand why not.

I'll have to dedicate some time to investigate with LD_DEBUG what's getting resolved incorrectly and why.

staticfloat · 2023-03-31T16:55:44Z

It seems more breaking to me not to correctly support resolving Julia-exported symbols in the "local" runtime without a handle.

Well, I kind of agree with Jameson that the "no library ccall" form is kind of a mistake. I know we can't outright remove it from the language, but I'm more okay with saying that piece of functionality is not supported when running with multiple system images, since that's a somewhat new way of running Julia.

topolarity · 2023-03-31T17:12:04Z

It seems more breaking to me not to correctly support resolving Julia-exported symbols in the "local" runtime without a handle.

Well, I kind of agree with Jameson that the "no library ccall" form is kind of a mistake. I know we can't outright remove it from the language, but I'm more okay with saying that piece of functionality is not supported when running with multiple system images, since that's a somewhat new way of running Julia.

I guess.. But it's bad sign to me that only 3 of the 24 examples in the ccall docs would meet that requirement, since "library name as a string" wouldn't count either.

My two cents on the matter (which admittedly doesn't mean much) is that we should take an opportunity to support our usual feature-set for this use-case if we can. Anything less means dividing the ecosystem into "running Julia as a library works, except..." and fragmenting the Julia ecosystem into packages that have/been tested/optimized for a particular use case and those that haven't.

I know that's already the reality of our statically-compiled ecosystem, but I think it's important to make positive progress towards provide better (simpler) guarantees about supported features, not splitting support across more use cases (even if they are arguably "niche").

staticfloat · 2023-03-31T17:26:23Z

since "library name as a string" wouldn't count either.

Sorry if I'm being dense, but why wouldn't providing the library name work? Are we expecting to have multiple libraries with the same name but different symbol versions loaded?

topolarity · 2023-03-31T18:15:31Z

Sorry if I'm being dense, but why wouldn't providing the library name work? Are we expecting to have multiple libraries with the same name but different symbol versions loaded?

I think it depends on what you mean by name. In the case of Julia-in-Julia, the SONAME of the libjulia's that we're loading simultaneously is unique, but each version still has a symlink "libjulia-internal.so" that in principle could be found by jl_load_dynamic_library and is valid as a ccall with the library "libjulia-internal". In that case, we're depending on the search order of jl_load_dynamic_library to resolve to the correct handle for us in each separate run-time.

I'm not sure whether that works out-of-the-box or requires some extra resolution shims to handle correctly, but otherwise yeah I think we could make deprecate only ccall(:symbol) and leave ccall((:symbol, :library), ... working.

If we're ready to deprecate that functionality across the board, then I'm happy to leave it out. My main point is to support the feature at least until we're ready to take that step, since at the end of the day it's a similar resolution shim.

vtjnash · 2023-03-31T19:22:45Z

I think intercepting the “no library” case and injecting libjulia first is good. It is what we require on Windows and for ijl_ symbols already anyways.

topolarity · 2023-04-10T17:46:01Z

Okay, good news! I was able to get the Windows-style lookup-interception working. The new change looks like:

Look-up and store the libjulia.so handle at start-up
Add an equivalent to jl_dlfind_win32 (to search in the "local" libjulia before searching executable-wide)

One caveat is that relying on the (non-)existence of a symbol for, e.g., version detection does not work. Any look-up that fails against the "local" libjulia can still succeed when continuing to the global symbol resolution.

If that approach sounds good to you @vtjnash I can update this PR. #49012 would be essentially unchanged, since the symbol versioning is required anyway to get the DT_NEEDED symbol resolution correct.

src/julia_internal.h

src/dlload.c

This is similar to what we do on Windows, where handles to the exact library are required anyway. This is enough to make sure that our `dlsym` lookups are directed to the correct libjulia, even when loading a Julia run-time within Julia. The second change needed to get things working (not included in this commit) is to add symbol versioning, so that the runtime linker does not mix up symbols between the two libraries.

This prevents the library from being upgraded to the global namespace for symbol resolution. We were already making this mistake before this function was introduced, but probably did not notice it because RTLD_LOCAL is the default behavior on Linux. On the other hand, macOS does need this fix.

src/dlload.c

vtjnash

RSLGTM at Elliot's request

maleadt · 2023-05-03T07:32:50Z

It would probably have been good to run PkgEval here? The PR has broken CBinding.jl, which is used by several packages (BPFNative.jl, System.jl, XXhash.jl, PLCTag.jl, ...):

[74] signal (11.1): Segmentation fault
in expression starting at none:1
jl_isabspath at /cache/build/default-amdci4-2/julialang/julia-master/src/init.c:536
ijl_load_dynamic_library at /cache/build/default-amdci4-2/julialang/julia-master/src/dlload.c:278
#dlopen#3 at ./libdl.jl:117 [inlined]
dlopen at ./libdl.jl:116 [inlined]
dlopen at ./libdl.jl:116 [inlined]
configure! at /home/pkgeval/.julia/packages/CBinding/PrYH5/src/context.jl:293
Context at /home/pkgeval/.julia/packages/CBinding/PrYH5/src/CBinding.jl:149

CBinding is calling dlopen(NULL) in order to get a handle to libjulia, and that functionality has been removed int his PR. https://github.com/analytech-solutions/CBinding.jl/blob/38e76e51d31e22beaa125f7fb9928d0cd1057e15/src/context.jl#L242-L244 https://github.com/analytech-solutions/CBinding.jl/blob/38e76e51d31e22beaa125f7fb9928d0cd1057e15/src/context.jl#L293-L294

vtjnash · 2023-05-03T10:27:01Z

Okay, but "this is a risky hack" does not inspire much confidence. We do not support that because it is not correct to specify that on Windows, so it is not cross-platform compliant.

vtjnash · 2023-05-03T10:31:09Z

src/dlload.c

@@ -255,26 +275,6 @@ JL_DLLEXPORT void *jl_load_dynamic_library(const char *modname, unsigned flags,
    int n_extensions = endswith_extension(modname) ? 1 : N_EXTENSIONS;
    int ret;

-    /*
-      this branch returns handle of libjulia-internal


I think it was actually trying to trigger this case however, which is not as much of a trick and not quite what it claimed to be doing. I assume we could put this back?

Looks like it, yes. Putting it back makes CBinding work again, as expected. @topolarity what was the reason to remove this functionality?

I made a mistake and assumed that downstream code would not end up here after all of the runtime-internal uses were re-factored into jl_find_library_by_addr.

PR open to fix: #49611

KristofferC · 2023-05-08T07:38:44Z

Can I get more info on why this should go into 1.9? Is there something that is blocked waiting on it?

topolarity marked this pull request as ready for review March 15, 2023 13:39

topolarity force-pushed the dlvsym-ccall branch from 93580e2 to 02a7ec1 Compare March 15, 2023 15:04

topolarity mentioned this pull request Mar 15, 2023

Makefile: Add support for symbol versioning (ON by default) #49012

Merged

topolarity added the backport 1.9 Change should be backported to release-1.9 label Mar 15, 2023

staticfloat reviewed Mar 15, 2023

View reviewed changes

topolarity force-pushed the dlvsym-ccall branch 5 times, most recently from f32da50 to d1cf142 Compare March 16, 2023 14:59

staticfloat approved these changes Mar 16, 2023

View reviewed changes

staticfloat requested a review from vtjnash March 16, 2023 15:14

KristofferC mentioned this pull request Mar 24, 2023

Backports for 1.9.0-rc2 #48935

Merged

52 tasks

topolarity self-assigned this Apr 5, 2023

KristofferC mentioned this pull request Apr 9, 2023

Backports for 1.9.0 #49302

Merged

26 tasks

topolarity force-pushed the dlvsym-ccall branch from d1cf142 to 7c64549 Compare April 11, 2023 17:33

topolarity changed the title ~~Add internal dlvsym support for ccall's~~ Lookup ccall symbols in internal libraries first Apr 11, 2023

vtjnash reviewed Apr 11, 2023

View reviewed changes

src/julia_internal.h Outdated Show resolved Hide resolved

src/dlload.c Outdated Show resolved Hide resolved

src/dlload.c Outdated Show resolved Hide resolved

topolarity force-pushed the dlvsym-ccall branch from 7c64549 to ef65ab5 Compare April 11, 2023 17:58

topolarity force-pushed the dlvsym-ccall branch from ef65ab5 to 5d7cb70 Compare April 12, 2023 17:12

topolarity force-pushed the dlvsym-ccall branch from 5d7cb70 to 95f4376 Compare April 13, 2023 14:33

topolarity requested a review from vtjnash April 17, 2023 16:49

vtjnash reviewed Apr 18, 2023

View reviewed changes

src/dlload.c Outdated Show resolved Hide resolved

Merge branch 'master' into dlvsym-ccall

d22ca94

vtjnash reviewed May 1, 2023

View reviewed changes

staticfloat merged commit d82ae9e into JuliaLang:master May 1, 2023

vtjnash reviewed May 3, 2023

View reviewed changes

maleadt mentioned this pull request May 3, 2023

dlopen(NULL) returns a handle for libjulia-internal, not the process analytech-solutions/CBinding.jl#120

Open

KristofferC mentioned this pull request May 8, 2023

Backports for julia 1.9.1 #49680

Merged

51 tasks

KristofferC mentioned this pull request Jun 6, 2023

Backports for 1.9.2 #50090

Merged

36 tasks

topolarity removed the backport 1.9 Change should be backported to release-1.9 label Jun 6, 2023

vtjnash mentioned this pull request Aug 30, 2023

cglobal: Add an option to bypass jlplt mechanism #51108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lookup `ccall` symbols in internal libraries first #49010

Lookup `ccall` symbols in internal libraries first #49010

topolarity commented Mar 15, 2023 •

edited

Loading

staticfloat left a comment •

edited

Loading

topolarity commented Mar 15, 2023

Pangoraw commented Mar 16, 2023 •

edited

Loading

topolarity commented Mar 16, 2023

staticfloat left a comment

staticfloat commented Mar 30, 2023

topolarity commented Mar 31, 2023

staticfloat commented Mar 31, 2023

topolarity commented Mar 31, 2023

staticfloat commented Mar 31, 2023

topolarity commented Mar 31, 2023

vtjnash commented Mar 31, 2023

topolarity commented Apr 10, 2023

vtjnash left a comment

maleadt commented May 3, 2023

vtjnash commented May 3, 2023

vtjnash May 3, 2023

maleadt May 3, 2023

topolarity May 3, 2023

KristofferC commented May 8, 2023

Lookup ccall symbols in internal libraries first #49010

Lookup ccall symbols in internal libraries first #49010

Conversation

topolarity commented Mar 15, 2023 • edited Loading

staticfloat left a comment • edited Loading

Choose a reason for hiding this comment

topolarity commented Mar 15, 2023

Pangoraw commented Mar 16, 2023 • edited Loading

topolarity commented Mar 16, 2023

staticfloat left a comment

Choose a reason for hiding this comment

staticfloat commented Mar 30, 2023

topolarity commented Mar 31, 2023

staticfloat commented Mar 31, 2023

topolarity commented Mar 31, 2023

staticfloat commented Mar 31, 2023

topolarity commented Mar 31, 2023

vtjnash commented Mar 31, 2023

topolarity commented Apr 10, 2023

vtjnash left a comment

Choose a reason for hiding this comment

maleadt commented May 3, 2023

vtjnash commented May 3, 2023

vtjnash May 3, 2023

Choose a reason for hiding this comment

maleadt May 3, 2023

Choose a reason for hiding this comment

topolarity May 3, 2023

Choose a reason for hiding this comment

KristofferC commented May 8, 2023

Lookup `ccall` symbols in internal libraries first #49010

Lookup `ccall` symbols in internal libraries first #49010

topolarity commented Mar 15, 2023 •

edited

Loading

staticfloat left a comment •

edited

Loading

Pangoraw commented Mar 16, 2023 •

edited

Loading