Hide private symbols in libarchive.so

[yugr]

Libarchive.so presently exports 281 symbols (over 50%, full list attached) which are not present in libarchive's headers and thus are not supposed to be used by clients.

Removing these symbols would allow compiler to optimize code more aggressively (.text reduced by 1%), 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?

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

The issue was found using ShlibVisibilityChecker.

[kientzle]

First, you should double-check your list. Libarchive's public API includes everything in archive.h and everything in archive_entry.h. It looks like you've ignored the second of these files.

A 1% reduction in code size is not particularly interesting, and I suspect the dynamic linker speedup is similarly modest.

However, preventing clients from using internal APIs is a good thing, and this seems like a simple way to achieve that.

[yugr]

Thanks, Tim. Indeed, I missed archive_entry.h, with it removed symbol count dropped to 129. I'm attaching the updated patch.

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

Note that code size reduction is a bit misleading. Hidden visibility allows to perform inlining and cloning more aggressively so code size typically grows at -O2. Disasm shows that GCC did function splitting on __rhiv_nsur_lox_lg, __rhiv_r_iltr_h, __rhiv_r_iltr_onsum and __rhiv_writ_iltr which should hopefully improve runtime performance (LTO results would be more impressive due to cross-module inlining).

BTW do you think -fno-semantic-interposition is worth a try? This would allow GCC to assume that even exported functions won't be interposed at runtime and optimize calls to them more heavily.

[yugr]

Tim, have you made up your mind on this? I think it it's a no-go we better close the issue, otherwise I'd be happy to perform additional testing/refactor.

Updated patch which does a better job of checking for availability of visibility.

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

[yugr]

@kientzle A friendly reminder. I'm fine with closing this if it's not interesting for the project. If it is, I'd be happy to make needed updates.

[nanonyme]

It would be really useful to have this. Exposing private symbols makes it very hard for ABI checkers as well to function correctly.

[kientzle]

I have no problems with this.

[nanonyme]

@yugr did you validate that building against this project still works after your change? (In other words public ABI remains such)

[yugr]

@yugr did you validate that building against this project still works after your change? (In other words public ABI remains such)

AFAIR I only verified that make check worked after my change. Do you suggest to build few projects that depend on libarchive to see that there are no issues?

[nanonyme]

I took liberty of grabbing patch for testing into https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/merge_requests/7646, it will be built against bunch of projects and ABI checker against version without the patch will be ran at the end of build which should clearly state which symbols get hidden.

[nanonyme]

abidiff.txt
Okay, everything looked fine, here is the ABI diff. Feel free to validate nothing unintended got removed.

[kientzle]

Two concerns:

• The abidiff shows a lot of archive_entry symbols getting dropped that should not be
• I believe a lot of the PPmd8 symbols should be static (I see, this file is imported verbatim from an upstream source, so we probably shouldn't modify it.)

[nanonyme]

Okay, I would say fixing visibility of archive_entry symbols is probably blocker for this work then. Maybe they should be exposed through headers?

[yugr]

• The abidiff shows a lot of archive_entry symbols getting dropped that should not be

These symbols are not present in installed headers so shouldn't this be a good thing?

[nanonyme]

@kientzle so what's the next step? Are those actually supposed to be exposed and if yes, should they be added into headers?

[nanonyme]

This ABI cleanup would still be desired by libarchive downstreams.

[kientzle]

I was mistaken. Those are not in fact supposed to be exposed. This can go ahead. (Sorry for the long delay.)

[yugr]

Fixed.