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.

Sign up for GitHub

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

Jump to bottom

[Feature #20005] Add C API to resolve symbols from other extensions #8991

Merged
merged 2 commits into from Dec 14, 2023
Merged

[Feature #20005] Add C API to resolve symbols from other extensions #8991

merged 2 commits into from Dec 14, 2023

Conversation

tagomoris
Copy link
Contributor

This is to add a C API for extensions to resolve symbols extern-ed by other extensions.

https://bugs.ruby-lang.org/issues/20005

@tagomoris
Copy link
Contributor Author

I have no idea how we can test this kind of API. What's the best example?

@tagomoris
Copy link
Contributor Author

I got some idea about ext/-test- and test/-ext-. So I'll try them anyway.

@KJTsanaktsidis
Copy link
Contributor

That sounds right - make two modules in ext/-test-, and make one try and resolve symbols from the other. I think the feature name for an extension foo in ext/-test-/foo will be -test-/foo

@tagomoris
Copy link
Contributor Author

I added extensions for tests under ext/-test- and also added a test case under text/-ext-. These look working correctly on my laptop.

@tagomoris tagomoris marked this pull request as ready for review November 23, 2023 13:39
nobu
nobu requested changes Nov 24, 2023
View reviewed changes
dln.c Outdated Show resolved Hide resolved
load.c Outdated Show resolved Hide resolved
load.c Outdated Show resolved Hide resolved
load.c Outdated Show resolved Hide resolved
load.c Outdated Show resolved Hide resolved
load.c Outdated Show resolved Hide resolved
load.c Outdated Show resolved Hide resolved
tagomoris
tagomoris commented Nov 26, 2023
View reviewed changes
dln.c
memcpy(tmp + sizeof(EXTERNAL_PREFIX) - 1, symbol, symlen + 1);
symbol = tmp;
}
if (handle == NULL) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API to pass NULL as the handle argument looks dirty - should I add another argument to search the main executable explicitly?

tagomoris
tagomoris commented Nov 26, 2023
View reviewed changes
dln.c Outdated Show resolved Hide resolved
@tagomoris
Copy link
Contributor Author

CIs are failing on Windows environments. These show rb_ext_resolve_symbol is failing to resolve symbols. Hmm?

@tagomoris
Copy link
Contributor Author

And YJIT-ed runs crashed.

@tagomoris
Copy link
Contributor Author

tagomoris commented Nov 28, 2023 •
edited

About the CIs on Windows environments, the LoadError message <LoadError: resolve_symbol_target is not loaded: 80> explains that GetProcAddress(handle, symbol); returns NULL with a valid handle (not statically linked).

Follow-up: I got the error message from GetProcAddress: #<LoadError: 127: The specified procedure could not be found. - rst_any_method>

@tagomoris
Copy link
Contributor Author

I thought it was enough to specify RUBY_EXTERN to export a symbol from a .dll, but failing tests show it's not. Does it need to have *.def to declare EXPORTS explicitly? Or is there anything I'm missing?
I'll investigate more later.

@tagomoris tagomoris force-pushed the resolve-feature-symbols branch 2 times, most recently from 9b17a50 to a01335e Compare November 30, 2023 03:56
@tagomoris
Copy link
Contributor Author

All tests passed!

@tagomoris
Copy link
Contributor Author

I've updated my commits and pushed again.
@nobu Could you review this change once more?

@tagomoris tagomoris force-pushed the resolve-feature-symbols branch 2 times, most recently from dd162ac to 367e2d6 Compare December 8, 2023 06:34
@tagomoris
dln_symbol: make dln_sym accessible Ruby internally
5697b57 
The symbol resolved by dln_symbol will eventually be passed to
extensions. The error handling of dln_sym is also separated into
dln_sym_func because the new call resolving symbols will not raise
LoadError.
@tagomoris
rb_ext_resolve_symbol: C API to resolve and return externed symbols […
17faaad 
…Feature #20005]

This is a C API for extensions to resolve and get function symbols of other extensions.
Extensions can check the expected symbol is correctly loaded and accessible, and
use it if it is available.
Otherwise, extensions can raise their own error to guide users to setup their
environments correctly and what's missing.
@tagomoris
Copy link
Contributor Author

Can this get merged into Ruby 3.3?

@nobu nobu merged commit e51f9e9 into ruby:master Dec 14, 2023
98 checks passed
@tagomoris tagomoris deleted the resolve-feature-symbols branch December 15, 2023 01:40
@tagomoris tagomoris mentioned this pull request Dec 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nobu nobu nobu approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@tagomoris @KJTsanaktsidis @nobu