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

Please don't export private symbols #130

Closed
wRAR opened this issue Oct 21, 2017 · 8 comments
Closed

Please don't export private symbols #130

wRAR opened this issue Oct 21, 2017 · 8 comments

Comments

@wRAR
Copy link
Contributor

@wRAR wRAR commented Oct 21, 2017

It seems there are a lot of symbols which are private and not a part of the public ABI that are exported. Those include _hashtable_*, blake2b_ (when the bundled b2 is used), and probably some of rs_* functions (at least some of rs_* functions were exported in 1.0.0 but are not exported anymore because they were made static but SONAME was not changed so I assume they were private from the beginning).
Setting all symbols as hidden and only explictitly marking public ones as visible helps maintaining ABI correctly and understanding when the SONAME must be bumped.

@dbaarda
Copy link
Member

@dbaarda dbaarda commented Oct 23, 2017

The intention has always been "if it's not in librsync.h, it's not public". I realize that this doesn't really hide them from the linker, and the TODO.md includes "Rename all symbols that are intended to be private to rs__" as a way to try and avoid name collisions when librsync is linked into other projects.

I'm not entirely sure how to make them properly private. The problem is the implementation is spread out over multiple *.c files that need to call these "private" functions in each other. For example, the state machine driver is implemented in job.c, but the state functions for each operation type are defined in their own delta.c/mksum.c/etc files. Some of these *.c files are stand-alone reusable components (like hashtable or rollsum) or come from upstream sources (like blake2). We'd have to extensively patch upstream sources with "static"s all over the place and then they couldn't call each other over the *.c file module boundaries.

The only way I can think to hide these is to either merge them all into one monolithic *.c file, or do the equivalent and have a main librsync.c file that #includes all the *.c files. Is there some other standard way to do this in C that I'm unaware of? Is there some best-practice code structure we could adopt to fix this?

We can probably improve things without major restructuring by just doing the TODO renaming and making sure everything that can be is marked static. Other than that, I'm open to suggestions.

@wRAR
Copy link
Contributor Author

@wRAR wRAR commented Oct 23, 2017

@dbaarda
Copy link
Member

@dbaarda dbaarda commented Oct 24, 2017

That's C++, not C. It also looks terrifyingly compiler dependent.

@wRAR
Copy link
Contributor Author

@wRAR wRAR commented Oct 24, 2017

It's not C++- specific, and other compilers have other ways to do that (like __declspec(dllexport) for MSVC).

@michihenning
Copy link

@michihenning michihenning commented Oct 25, 2017

https://stackoverflow.com/questions/19418908/creating-a-portable-library-to-run-on-both-linux-and-windows

The #defines suggested in some of the answer there are widely used.

@dbaarda
Copy link
Member

@dbaarda dbaarda commented Nov 7, 2017

I had a bit of a read, and IMHO if we are going to do this right, using the CMake support for it would be the best approach to address the compiler/platform differences;

https://cmake.org/cmake/help/v3.0/module/GenerateExportHeader.html

I would still like to avoid any significant changes to upstream sources like blake2, so I would prefer not to have to annotate it with special LIBRSYNC_NO_EXPORT macro's everywhere. However, perhaps there is a way to make it not export by default so we only have to add LIBRSYNC_EXPORT to the public bits of librsync?

I think its still also worth doing a cleanup pass first to tag everything possible as "static", perhaps doing some re-arrangements to enable more of that, and make sure that everything is consistently named with rs_* prefixes.

@wRAR
Copy link
Contributor Author

@wRAR wRAR commented Nov 12, 2017

I suppose set(CMAKE_CXX_VISIBILITY_PRESET hidden) (from the GenerateExportHeader docs) does precisely "make it not export by default" (i.e. -fvisibility=hidden).

dbaarda added a commit to dbaarda/librsync that referenced this issue Aug 16, 2019
dbaarda added a commit to dbaarda/librsync that referenced this issue Aug 16, 2019
This is not relevant now we have fixed librsync#130 exporting private symbols.
@dbaarda
Copy link
Member

@dbaarda dbaarda commented Aug 16, 2019

wRAR: I see that v2.0.2 has made it into Debian testing, and I'm pleased to see how many other packages are already using/depending on it. Thank you very much for all your work there.

Can you have a quick look at #156 and comment from a Debian packaging point of view to make sure this doesn't cause any headaches for you? I'm keen to release v2.0.3 with this included to address this and the memory leak bug that have been fixed since v2.0.2 was released.

dbaarda added a commit that referenced this issue Aug 17, 2019
Fix #130 don't export private symbols.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants