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

Two-level namespace #34

Closed
wants to merge 6 commits into from
Closed

Two-level namespace #34

wants to merge 6 commits into from

Conversation

kastiglione
Copy link
Contributor

@kastiglione kastiglione commented Sep 8, 2016

This change adds support for rebinding that leverages mach-o's two-level namespace.

The tl;dr of two-level namespaces is that symbols imported from other libraries will reference which library they came from. When rebinding a symbol by name alone, it's possible to unintentionally rebind symbols that happen to have the same name, which can cause unexpected results. Using the two-level namespace prevents the mysterious bugs and crashes that can happen when rebinding by just symbol name alone.

In addition to correctness, this change opens a new performance optimization, see 7cc0515.

This change is a breaking change: callers of rebind_symbols now need to fill in a dylib field on the rebinding struct. This field can be NULL to opt-out of two-level namespaces, and enable the previous global search.

Resolves #32

@ghost ghost added the CLA Signed label Sep 8, 2016
@kastiglione kastiglione changed the title Two level namespace Two-level namespace Sep 8, 2016
@kastiglione
Copy link
Contributor Author

I've cleaned this up, hopefully this can get a reviewer or two. @grp do you have any thoughts on this, either in general or on the code?

@grp
Copy link
Contributor

grp commented Sep 10, 2016

I don't know much about the implementation of two-level namespacing. :( It looked cool reading through it, though.

@kastiglione
Copy link
Contributor Author

I'll review this in a ~week, hopefully that will give me somewhat fresh eyes to catch bugs, issues, readability problems, etc. I'd like to write a few more tests too (though I probably won't check the tests in).

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email [email protected] with your details so we can update your status.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@jobsyu jobsyu mentioned this pull request Jun 18, 2020
@kastiglione kastiglione closed this Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support two-level namespace
3 participants