Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

ELF Visibility #647

Merged
merged 2 commits into from
Feb 19, 2018
Merged

ELF Visibility #647

merged 2 commits into from
Feb 19, 2018

Conversation

ascent12
Copy link
Member

This explicitly exports API functions, and sets the default visibility to hidden.
It's a pretty typical and recommended thing for libraries to do.

Yes, it requires a GNU extension (appropriately hidden behind a feature test macro), but I believe this is a justified exception.

Compiled with -Dbuildtype=minsize -Db_lto=true and opening rootston
With patch:

     22109:	
     22109:	runtime linker statistics:
     22109:	  total startup time in dynamic loader: 27169614 cycles
     22109:		    time needed for relocation: 15428710 cycles (56.7%)
     22109:	                 number of relocations: 4717
     22109:	      number of relocations from cache: 2556
     22109:	        number of relative relocations: 16210
     22109:		   time needed to load objects: 10641900 cycles (39.1%)

436K libwlroots.so.0.0.0

Without patch:

     22987:	
     22987:	runtime linker statistics:
     22987:	  total startup time in dynamic loader: 42604509 cycles
     22987:		    time needed for relocation: 25563909 cycles (60.0%)
     22987:	                 number of relocations: 4763
     22987:	      number of relocations from cache: 2560
     22987:	        number of relative relocations: 16194
     22987:		   time needed to load objects: 15456870 cycles (36.2%)

460K libwlroots.so.0.0.0

As you can see, it makes loading libwlroots a fair bit faster, and makes the final binary a little bit smaller.
It may also be faster, because the LTO can do more, but I have not benchmarked this.

@ddevault
Copy link
Contributor

On my 3.5 GHz CPU, using your numbers, this would save us about 0.001 seconds. I've said no to this before and I'm going to say no to it again.

@ddevault ddevault closed this Feb 19, 2018
@ascent12
Copy link
Member Author

Part of it is also not accidentally exporting functions or running into symbol collisions.
If a library user defines a function with the same name as one of our internal functions, they may run into linker issues.

Another example is os_create_anonymous_file. Our examples are reaching into the library internals, and using a function that we don't have in our headers.
That's just asking for trouble.

This creates a much more explicit and clean separation for the library. There is a good reason that like 99% of libraries do this.

@ddevault
Copy link
Contributor

Propose it for standardization. Until then, no.

@ascent12
Copy link
Member Author

It's part of the ELF standard.

@ddevault
Copy link
Contributor

Halfway there!

@ascent12
Copy link
Member Author

ascent12 commented Feb 19, 2018

Do you have an actual technical reason for rejecting this?
I'm not going to accept shit without justification.

Yes, non-standard shit isn't good, but it's hidden behind a feature test macro, and we've already have a __GNUC__ test in log.h since basically the very beginning.

@ddevault
Copy link
Contributor

I don't use nonstandard features. That test in log.h really bothers me, I shouldn't have agreed to it.

@ascent12
Copy link
Member Author

The benefits outweigh the consequences. The only tangible downside is a "bad feeling".
There are no effects on the semantics of the code, so any compiler not compatible with GCC will handle it just fine.

The log.h one gives us extra warnings, which prevents a whole bunch of pointless bugs.
This one removes the ability for people to fuck with library internals, and allows an LTO to work better.

@ddevault
Copy link
Contributor

No, the tangible downside is encourange the use of and proliferation of nonstandard features, which I am staunchly against and always have been. You know how this PR was going to go down. Why did you even bother? Just to create animosity?

@ascent12
Copy link
Member Author

ascent12 commented Feb 19, 2018

I think we need to remove the pretence that our code is portable and standards conforming. There is all sorts of VERY Linux-specific shit we're doing, but you're not complaining because instead of being behind __GNUC__, they're being disabled by __FreeBSD__. I just don't think this "absolutely portable" argument holds any weight.

We're even using other non-standard shit like ## comma deletion and empty variadic macros. Maybe it'll be more relevant when -Wpedantic comes up clean.

I'm not saying that we should go around making everything non-portable; our effort to make things more portable made it even possible for this to run on FreeBSD. I just think there are certain exceptions which are justified, because they provide extremely useful features. Especially things as benign as this and __attribute__((printf)).

@ddevault
Copy link
Contributor

There are several kinds of portability. There's OS portability, libc portability, and compiler portability, to name a few important ones. The nature of this beast is that we aren't gonna get OS portability simply because the whole point is to leverage OS-specific features. But I'll be damned if we're gonna give up on libc or compiler portability, too.

We're even using other non-standard shit like ## comma deletion and empty variadic macros. Maybe it'll be more relevant when -Wpedantic comes up clean.

Let's file tickets and get this fixed.

@ascent12
Copy link
Member Author

That is going to break probably 50% of our calls to wlr_log, and make it way harder to use.

@ddevault
Copy link
Contributor

We don't need to change wlr_log right away. The value-add of that one is much higher than of ELF visibility, so it's easier for me to look the other way.

@ascent12
Copy link
Member Author

But I'll be damned if we're gonna give up on libc or compiler portability, too

That's why feature test macros exist.

The value-add of that one is much higher than of ELF visibility, so it's easier for me to look the other way.

Sure, this isn't the most significant of improvements, but preventing
https://github.com/swaywm/wlroots/blob/master/examples/screenshot.c#L129
is more than enough reason for me.

Funnily enough, you can't even hide ## behind macros, but you can for this. It's quite the double standard.

@ddevault ddevault reopened this Feb 19, 2018
@ddevault ddevault merged commit 868ad5a into swaywm:master Feb 19, 2018
@ddevault
Copy link
Contributor

I was willing to consider dropping warnings for wlr_log, but we need to use ## for the filename and line number which is a really damn compelling feature. If we're going to let it slide a little, then we might as well do this too.

But let the record state that I think this sucks.

@agx
Copy link
Contributor

agx commented Feb 19, 2018

What about using a symbols file here instead:

ftp://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_25.html

or in actual use:

https://github.com/libvirt/libvirt/blob/master/src/libvirt_admin_private.syms
https://github.com/libvirt/libvirt/blob/master/src/libvirt_admin_public.syms

This prevents cluttering the C file and is supported by GNUs ld as well as lld.

https://releases.llvm.org/3.9.0/tools/lld/docs/ReleaseNotes.html#symbol-versioning

We could simply make wlr_* public and everything else private and at a later point introduce actual versions if needed.

@emersion
Copy link
Member

I wonder if extern would be useful here.

@ddevault
Copy link
Contributor

That sounds better to me, agx.

@ddevault ddevault mentioned this pull request Feb 19, 2018
@ascent12 ascent12 deleted the elf_visibility branch November 18, 2018 00:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants