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

Define YP_EXPORTED_FUNCTION as empty #1111

Closed
wants to merge 1 commit into from

Conversation

tenderlove
Copy link
Member

@tenderlove tenderlove commented Jun 30, 2023

We are currently leaking YARP symbols in Ruby 3.3 (Ruby head):

$ nm libruby.3.3-static.a | grep yp_parser_init
000000000000000c T _yp_parser_init
000000000002a393 s l___func__.yp_parser_init

This is not good because if we use Ruby 3.3 to compile YARP as a C
extension (for example installing the YARP gem), the linker gets
confused and ends up prioritizing the symbol found in Ruby rather than
the one from the Gem. This means the C extension can end up calling in
to yp_parser_init in Ruby rather than the new one shipped with the
extension.

If I use current Ruby head to compile YARP in this repository, nm will
show that yp_parser_init is not defined:

$ nm lib/yarp.bundle | grep yp_parser_init
                 U _yp_parser_init

This is because the linker got confused about the leaked symbols.

I would like to default all functions to not be external. This hides
the symbols and fixes the problem with Ruby and YARP as a gem. If I
apply this patch to Ruby, build Ruby, then build YARP, yp_parser_init
is defined in the Gem:

$ nm lib/yarp.bundle | grep yp_parser_init
00000000000162f8 t _yp_parser_init
000000000002d8ac t _yp_parser_init.cold.1
000000000002d8d4 t _yp_parser_init.cold.2

/cc @flavorjones

Related to #1110

We are currently leaking YARP symbols in Ruby 3.3 (Ruby head):

```
$ nm libruby.3.3-static.a | grep yp_parser_init
000000000000000c T _yp_parser_init
000000000002a393 s l___func__.yp_parser_init
```

This is not good because if we use Ruby 3.3 to compile YARP as a C
extension (for example installing the YARP gem), the linker gets
confused and ends up prioritizing the symbol found in Ruby rather than
the one from the Gem.  This means the C extension can end up calling in
to `yp_parser_init` in Ruby rather than the new one shipped with the
extension.

If I use current Ruby head to compile YARP in this repository, `nm` will
show that `yp_parser_init` is *not* defined:

```
$ nm lib/yarp.bundle | grep yp_parser_init
                 U _yp_parser_init
```

This is because the linker got confused about the leaked symbols.

I would like to default all functions to *not* be external.  This hides
the symbols and fixes the problem with Ruby and YARP as a gem.  If I
apply this patch to Ruby, build Ruby, then build YARP, `yp_parser_init`
is defined in the Gem:

```
$ nm lib/yarp.bundle | grep yp_parser_init
00000000000162f8 t _yp_parser_init
000000000002d8ac t _yp_parser_init.cold.1
000000000002d8d4 t _yp_parser_init.cold.2
```
@kddnewton
Copy link
Collaborator

I think at this point with this PR you can just remove the whole macro. If the point is that nothing should be exported let's get rid of the whole thing and just keep visibility hidden.

@eregon
Copy link
Member

eregon commented Jul 2, 2023

We need the symbols to be exported for the dynamic library, for Fiddle/FFI: #1112

@eregon
Copy link
Member

eregon commented Jul 2, 2023

Could Ruby define YP_STATIC to avoid exporting the symbols? Or use the librubyparser.a?

@flavorjones
Copy link
Contributor

flavorjones commented Jul 2, 2023

Yeah, just for the record: #1069 set up compilation so that compiling with -DYP_STATIC would hide symbols. This is how librubystatic.a is being built on yarp:main right now.

It seems like we have two options:

  1. downstream in Ruby, make sure YARP is compiled with -DYP_STATIC
  2. make static (hidden symbols) the default (what this PR chooses to do)

I don't care which; but if we choose 2 then we need to make sure librubyparser.so is being built so that symbols are shared, which this PR isn't doing:

$ nm build/librubyparser.so | fgrep yp_parse
0000000000021780 t yp_parse
0000000000021800 t yp_parse_serialize
00000000000216d0 t yp_parser_free
0000000000021440 t yp_parser_init
0000000000005620 t yp_parser_local_depth.isra.0
0000000000005550 t yp_parser_parameter_name_check
00000000000216b0 t yp_parser_register_encoding_changed_callback
00000000000216c0 t yp_parser_register_encoding_decode_callback

Compare that output to what gets built on main right now:

$ nm build/librubyparser.so | fgrep yp_parse
0000000000021830 T yp_parse
00000000000026d1 t yp_parse.cold
00000000000219f0 T yp_parse_serialize
0000000000021780 T yp_parser_free
00000000000214f0 T yp_parser_init
0000000000005820 t yp_parser_local_depth.isra.0
0000000000005750 t yp_parser_parameter_name_check
0000000000021760 T yp_parser_register_encoding_changed_callback
0000000000021770 T yp_parser_register_encoding_decode_callback

@eregon
Copy link
Member

eregon commented Jul 3, 2023

Yeah I'd be totally fine to change the default, just as you said the .so should have exported symbols, the rest shouldn't.

@tenderlove
Copy link
Member Author

2. make static (hidden symbols) the default (what this PR chooses to do)

Yes, I'd like to do this one because I don't really want to modify the build system in Ruby.

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>
@flavorjones
Copy link
Contributor

@tenderlove I think you can close this now that #1110 shipped.

@jemmaissroff
Copy link
Collaborator

Closing since #1110 shipped

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.

5 participants