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

Link with -Bsymbolic to make sure we use the local copy of houdini #32

Merged
merged 1 commit into from Apr 22, 2013

Conversation

tomhughes
Copy link
Contributor

If escape_utils is used with another gem (like redcarpet) that also has a copy of houdini inside then the dynamic linker can wind up using the wrong version of the houdini routines.

Adding -Bsymbolic to the linker flags tells the linker to prefer the local version when resolving symbols.

I assume this will actually need some guarding so that it only happens on ELF systems using the binutils linker but I have no idea how to achieve that...

@brianmario
Copy link
Owner

I think we actually want -fvisibility=hidden instead of this? /cc @vmg

@tomhughes
Copy link
Contributor Author

That would work as well, yes, so long as the symbol(s) that ruby needs to lookup when loading the module are still visible with dlsym when you do that?

@tomhughes
Copy link
Contributor Author

Yes, that works so long as you also add __attribute__((visibility("default"))) to Init_escape_utils so that the main entry point is visible to ruby.

@brianmario
Copy link
Owner

Yeah exactly

@tmm1
Copy link
Collaborator

tmm1 commented Apr 21, 2013

Note the visibility trick broke with rbx2. Would be nice to find a way to do and still stay compatible. /cc vmg/rinku#29

Also I've never heard of -Bsymbolic before. How portable is that?

@tomhughes
Copy link
Contributor Author

Well it adds DT_SYMBOLIC to the ELF dynamic section, so I assume it should work on any properly conformant ELF based system.

@vmg
Copy link
Collaborator

vmg commented Apr 21, 2013

I'm afraid this is worryingly not portable. The right way to go about this would be to hide the conflicting Houdini calls using attribute hidden in their declarations, instead of hiding everything by default.

@tomhughes
Copy link
Contributor Author

Really? Surely the the attributes are the non-portable thing and what you apply them to doesn't matter so much? or are you saying there are platforms where more than the main init routine needs to be exposed to ruby for a module to work?

In any case I don't really mind how it's fixed, but it would be nice to be able to load both gems at the same time ;-)

@vmg
Copy link
Collaborator

vmg commented Apr 21, 2013

Surely the the attributes are the non-portable thing and what you apply them to doesn't matter so much?

The attributes are a GCC extension, but an extension that works across all platforms. -Bsymbolic only works for ELF binaries. So in a sad, twisted way, the attributes are definitely more portable.

platforms where more than the main init routine needs to be exposed to ruby for a module to work?

Yes, that would be Rubinius. Maybe there is a way to export the RBX c extension identifier, but I'm not sure of how. Let me look into it.

@tomhughes
Copy link
Contributor Author

Well the easy way to add the attributes to just the houdini routines is probably to add pragmas around the houdini code:

#pragma GCC visibility push(hidden) 
...
#pragma GCC visibility pop

which should also avoid conflicts with non-gcc compilers which will likely ignore, or at most warn about, the pragma.

@vmg
Copy link
Collaborator

vmg commented Apr 22, 2013

I just got rubinius/rubinius#2302 merged, which means that the visibility trick with -Fvisibility now works on Rubinius too. Can you change this PR to use the visibility flag? Cheerios!

This avoids conflicts with other gems that may have some of the
same symbols, such as redcarpet which also uses houdini.
@tomhughes
Copy link
Contributor Author

Done. Also opened vmg/redcarpet#229 to make the same change to redcarpet.

@vmg
Copy link
Collaborator

vmg commented Apr 22, 2013

Brilliant. Thank you so much, Tom.

vmg pushed a commit that referenced this pull request Apr 22, 2013
Link with -Bsymbolic to make sure we use the local copy of houdini
@vmg vmg merged commit 3e0262c into brianmario:master Apr 22, 2013
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.

None yet

4 participants