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

fix static symbol resolution #1069

Merged

Conversation

flavorjones
Copy link
Contributor

@flavorjones flavorjones commented Jun 24, 2023

Closes #1067

This PR introduces separate object files (and build directories) for the dll (librubyparser.so) and the static archive (librubyparser.a) to ensure the dll makes public symbols public and the static archive does not.

I've also made the Makefile much quieter by default, but provide support for a V=1 argument for verbose mode (just like mkmf Makefiles).

This makes the difference between (uppercase T):

$ nm lib/yarp.so | fgrep yp_parse
0000000000031210 T yp_parse
0000000000003e1d t yp_parse.cold
00000000000313d0 T yp_parse_serialize
0000000000031160 T yp_parser_free
0000000000030ee0 T yp_parser_init
0000000000015520 t yp_parser_local_depth.isra.0
0000000000015450 t yp_parser_parameter_name_check
0000000000031140 T yp_parser_register_encoding_changed_callback
0000000000031150 T yp_parser_register_encoding_decode_callback

and (lowercase t):

$ nm lib/yarp.so | fgrep yp_parse
0000000000031050 t yp_parse
00000000000310d0 t yp_parse_serialize
0000000000030fa0 t yp_parser_free
0000000000030d20 t yp_parser_init
0000000000015200 t yp_parser_local_depth.isra.0
0000000000015130 t yp_parser_parameter_name_check
0000000000030f80 t yp_parser_register_encoding_changed_callback
0000000000030f90 t yp_parser_register_encoding_decode_callback

This is a prefactor so we can compile shared and static objects
separately in a subsequent commit.
but also offer a V=1 option for verbose mode
Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

Thank you!

@kddnewton kddnewton merged commit f59c375 into ruby:main Jun 24, 2023
@flavorjones flavorjones deleted the 1067-flavorjones-fix-static-symbol-resolution branch June 25, 2023 02:25
flavorjones added a commit to flavorjones/yarp that referenced this pull request Jul 9, 2023
This reverses the convention introduced in ruby#1069, and requires
`YP_EXPORT_SYMBOLS` to be defined at compile-time when building a
shared library.

See related ruby#1111 which describes a symbol leakage problem in Ruby
3.3 (ruby head) that this commit, once merged upstream into Ruby,
should address.

Note that the tests introduced in a previous commit ensure that the
static archive and shared objects are all being created correctly
after this change.
flavorjones added a commit to flavorjones/yarp that referenced this pull request Jul 9, 2023
This reverses the convention introduced in ruby#1069, and requires
`YP_EXPORT_SYMBOLS` to be defined at compile-time when building a
shared library.

See related ruby#1111 which describes a symbol leakage problem in Ruby
3.3 (ruby head) that this commit, once merged upstream into Ruby,
should address.

Note that the tests introduced in a previous commit ensure that the
static archive and shared objects are all being created correctly
after this change.

Co-authored-by: Aaron Patterson <tenderlove@ruby-lang.org>
flavorjones added a commit to flavorjones/yarp that referenced this pull request Jul 11, 2023
This reverses the convention introduced in ruby#1069, and requires
`YP_EXPORT_SYMBOLS` to be defined at compile-time when building a
shared library.

See related ruby#1111 which describes a symbol leakage problem in Ruby
3.3 (ruby head) that this commit, once merged upstream into Ruby,
should address.

Note that the tests introduced in a previous commit ensure that the
static archive and shared objects are all being created correctly
after this change.

Co-authored-by: Aaron Patterson <tenderlove@ruby-lang.org>
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.

Incorrect symbol resolution
2 participants