Page MenuHomePhabricator

[ELF] Add -Bsymbolic-non-weak-functions
ClosedPublic

Authored by MaskRay on May 15 2021, 11:02 PM.

Details

Summary

This option is a subset of -Bsymbolic-functions. It applies to STB_GLOBAL
STT_FUNC definitions.

The address of a vague linkage function (STB_WEAK STT_FUNC, e.g. an inline
function, a template instantiation) seen by a -Bsymbolic-functions linked
shared object may be different from the address seen from outside the shared
object. Such cases are uncommon. (ELF/Mach-O programs may use
-fvisibility-inlines-hidden to break such pointer equality. On Windows,
correct dllexport and dllimport are needed to make pointer equality work.
Windows link.exe enables /OPT:ICF by default so different inline functions may
have the same address.)

// a.cc -> a.o -> a.so (-Bsymbolic-functions)
inline void f() {}
void *g() { return (void *)&f; }

// b.cc -> b.o -> exe
// The address is different!
inline void f() {}

-Bsymbolic-non-weak-functions is a safer (C++ conforming) subset of
-Bsymbolic-functions, which can make the program work.

Implementations usually emit a vague linkage definition in a COMDAT group. We
could detect the group (with more code) but I feel that we should just check
STB_WEAK for simplicity. A weak definition will thus serve as an escape hatch
for rare cases when users want interposition on definitions.

GNU ld feature request: https://sourceware.org/bugzilla/show_bug.cgi?id=27871

Longer write-up: https://maskray.me/blog/2021-05-16-elf-interposition-and-bsymbolic

Diff Detail

Event Timeline

MaskRay created this revision.May 15 2021, 11:02 PM
MaskRay requested review of this revision.May 15 2021, 11:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2021, 11:02 PM
MaskRay edited the summary of this revision. (Show Details)May 15 2021, 11:03 PM
MaskRay edited the summary of this revision. (Show Details)May 15 2021, 11:07 PM

No objections from me, but we don't use symbol preemption in our downstream codebase, so I haven't got much experience with the implications one way or another, so probably best others give their points of view.

I don't have any strong objections. Just want to make sure that weak symbols are the right subset to use. My understanding is that this is coming from vague linkage https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague the only part that I could find in there about weak definitions is in static data: (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-static)

Some objects with static storage duration have associated guard variables used to ensure that they are initialized only once (see 3.3.3). If the object is emitted using a COMDAT group, the guard variable must be too. It is suggested that it be emitted in the same COMDAT group as the associated data object, but it may be emitted in its own COMDAT group, identified by its name. In either case, it must be weak.

The part about out of line copies of inline functions https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-inline says to define them in a comdat group but nothing about weak definitions. I think that both clang and GCC use weak definitions for functions in COMDAT groups but is that guaranteed of all compilers that might use LLD?

Some suggestions:

  • Is -Bsymbolic-non-vague-functions more descriptive, or perhaps -Bsymbolic-non-weak-functions? When I see global-functions as a programmer, not a linker person, I think of non-static functions rather than non-weak functions.
  • Would "Not defined in a COMDAT group" be a better criteria for the subset than non-weak? This would correctly find global symbols defined in COMDAT groups? Certainly more work to implement.
MaskRay updated this revision to Diff 345972.May 17 2021, 12:37 PM
MaskRay retitled this revision from RFC: [ELF] -Bsymbolic-global-functions to RFC: [ELF] -Bsymbolic-non-weak-functions.
MaskRay edited the summary of this revision. (Show Details)

-> -Bsymbolic-non-weak-functions

peter.smith accepted this revision.May 18 2021, 6:32 AM

Thanks for the update. I think it is a well defined useful subset of -Bsymbolic-functions so I'm happy to approve.

This revision is now accepted and ready to land.May 18 2021, 6:32 AM

Do you plan to land this? I thought of it as potentially useful for https://github.com/android/ndk/issues/1551. The issue has the full details, but the summary is that we only want a single copy of __emutls_get_address that's used by all shared objects in a process, otherwise different DSOs will see different emulated TLS state for the same variable. One way to ensure that would be to put the emulated TLS bits in their own DSO and have everyone else reference that, but a full DSO is pretty heavy-weight. Another would be to make it default visibility and weak and let the dynamic linker coalesce multiple copies at runtime; -Bsymbolic and -Bsymbolic-functions would break that, but -Bsymbolic-non-weak-functions wouldn't.

Do you plan to land this? I thought of it as potentially useful for https://github.com/android/ndk/issues/1551. The issue has the full details, but the summary is that we only want a single copy of __emutls_get_address that's used by all shared objects in a process, otherwise different DSOs will see different emulated TLS state for the same variable. One way to ensure that would be to put the emulated TLS bits in their own DSO and have everyone else reference that, but a full DSO is pretty heavy-weight. Another would be to make it default visibility and weak and let the dynamic linker coalesce multiple copies at runtime; -Bsymbolic and -Bsymbolic-functions would break that, but -Bsymbolic-non-weak-functions wouldn't.

I was waiting for users:) Glad that this can find a user.

For Linux distributions, I am hoping they can use a -fvisibility variant which applies to non-weak functions/variables (say, -fvisibility-non-weak=protected).
I think GNU people aren't thrilled at the idea (and so users are stuck with GCC -fpic -fsemantic-interposition suppressing lining).
Even with a compiler option, a linker option can be convenient as well. It allows easy experiments without recompilation.

MaskRay updated this revision to Diff 362889.Jul 29 2021, 2:41 PM
MaskRay retitled this revision from RFC: [ELF] -Bsymbolic-non-weak-functions to [ELF] -Bsymbolic-non-weak-functions.
MaskRay edited the summary of this revision. (Show Details)

rebase

This revision was landed with ongoing or failed builds.Jul 29 2021, 2:47 PM
This revision was automatically updated to reflect the committed changes.
MaskRay retitled this revision from [ELF] -Bsymbolic-non-weak-functions to [ELF] Add -Bsymbolic-non-weak-functions.Jul 29 2021, 2:47 PM

Thanks!

Btw, this is also pretty close to ld64's default behavior for Mach-O: non-weak definitions get bound directly, and weak definitions are left as interposable. The only difference is that ld64's behavior applies to data as well as functions.

Right:) The only reason that GNU ld needed -Bsymbolic-functions is to avoid collision with copy relocations.

In a -fpie/-fpic world, we should not see copy relocations from C/C++ code but GCC x86-64 still has the HAVE_LD_PIE_COPYRELOC issue😭 (https://gcc.gnu.org/legacy-ml/gcc/2019-05/msg00215.html) @hjl.tools

Android is considering this as their default behavior: https://github.com/android/ndk/issues/1552

Do you think it's worth cherry-picking this to LLVM 13, to get it more exposure? CC @tstellar

On our end, we build with full -Bsymbolic right now. Once we're testing with LLVM 13, I'll try -Bsymbolic-functions and -Bsymbolic-non-weak-functions and see if they make any meaningful performance or size differences for our apps (and how a hypothetical -Bsymbolic-non-weak, which would match default Mach-O behavior, might perform as well).

MaskRay added a comment.EditedJul 29 2021, 3:34 PM

Android is considering this as their default behavior: https://github.com/android/ndk/issues/1552

Do you think it's worth cherry-picking this to LLVM 13, to get it more exposure? CC @tstellar

LGTM. This is a very safe patch.

On our end, we build with full -Bsymbolic right now. Once we're testing with LLVM 13, I'll try -Bsymbolic-functions and -Bsymbolic-non-weak-functions and see if they make any meaningful performance or size differences for our apps (and how a hypothetical -Bsymbolic-non-weak, which would match default Mach-O behavior, might perform as well).

I also tested some internal server side applications. Some applications break with -Bsymbolic-functions but work with -Bsymbolic-non-weak-functions, so they do leverage function address pointer equality.

Adding -Bsymbolic-non-weak is also fine. I am always of the view that global variable accesses should not be in the performance bottleneck. Most applications should not observe any difference.
If an application sees improvement, it suggests some code problems. They can consider making the variable hidden (or protected if they don't use -fno-pic)


-fvisibility-non-weak=protected may benefit GCC (due to its -fsemantic-interposition for -fpic), but not clang.
That said, I do like using visibility to properly annotate object files

I've merged this to the release/13.x branch, so do we still need the release note in main?

MaskRay added a comment.EditedJul 29 2021, 7:33 PM

I've merged this to the release/13.x branch, so do we still need the release note in main?

The release note can be removed in main.

Do you plan to land this? I thought of it as potentially useful for https://github.com/android/ndk/issues/1551. The issue has the full details, but the summary is that we only want a single copy of __emutls_get_address that's used by all shared objects in a process, otherwise different DSOs will see different emulated TLS state for the same variable. One way to ensure that would be to put the emulated TLS bits in their own DSO and have everyone else reference that, but a full DSO is pretty heavy-weight. Another would be to make it default visibility and weak and let the dynamic linker coalesce multiple copies at runtime; -Bsymbolic and -Bsymbolic-functions would break that, but -Bsymbolic-non-weak-functions wouldn't.

I was waiting for users:) Glad that this can find a user.

I think I can deliver quite a few more :) I hadn't seen this patch before (thanks @smeenai for the heads up). It sounds like what Android would want to do by default.