search

cacalabs / libcaca

💩 Colour ASCII Art Library
Do What The F*ck You Want To Public License
519 stars 70 forks source link

Hide private symbols in libcaca #33

Closed yugr closed 6 years ago

yugr commented 6 years ago

Libcaca.so presently exports 134 symbols (35%, full list attached) which are not present in libcaca's headers and thus are not supposed to be used by clients.

Removing these symbols would allow compiler to optimize code more aggressively, speed up dynamic linker on Linux and prevent clients from inadvertently using internal APIs.

I attached a simple patch that hides private symbols. It passes make check (I can do additional testing if needed). Would something like this be interesting for the project?

caca_removed_syms.txt 0001-Hide-private-symbols.patch.txt

The issue was found using ShlibVisibilityChecker.

samhocevar commented 6 years ago

Yes, this is definitely interesting. Thanks for the analysis and patch. Following your report I have completely removed more than 100 of these symbols in 5f0ec215f8c9915ed028324a8ecac8212f68e18d (they were just weak aliases for backwards compatibility).

The only problem I can see with your patch is that it does not check whether the compiler understands -fvisibility=hidden. If you can fix this, I’ll consider it good to go. If you don’t feel like doing this extra work, I can have a look at it when I have the time.

yugr commented 6 years ago

The only problem I can see with your patch is that it does not check whether the compiler understands -fvisibility=hidden.

Thanks, Sam. TBH I tried to check for this (see the configure.ac hunk - it'll leave CACA_ENABLE_VISIBILITY undefined if gcc fails). Note that visibility annotations in headers are needed only when building libcaca (to make compiler assign correct visibility to them). Client code is fine with simple externs.

I attached new rebased patch (made Autoconf check more precise).

0001-Hide-private-symbols-v2.patch.txt

samhocevar commented 6 years ago

Oops, my apologies, I had misread the patch. It’s good to go! Do you want to do a PR or can I directly commit the changes?

yugr commented 6 years ago

Cool! I'm personally fine with plain commit (but I can do PR if that's preferred).

yugr commented 6 years ago

Sam, looks like I inadvertently disabled -fvisibility=hidden in last patch. Here's the fixed version.

0001-Hide-private-symbols-v3.patch.txt