This is an archive of the discontinued LLVM Phabricator instance.

[libunwind] unw_* alias fixes for ELF and Mach-O
ClosedPublic

Authored by rprichard on Dec 9 2020, 11:14 PM.

Details

Summary

Rename the CMake option, LIBUNWIND_HERMETIC_STATIC_LIBRARY, to
LIBUNWIND_HIDE_SYMBOLS. Rename the C macro define,
_LIBUNWIND_DISABLE_VISIBILITY_ANNOTATIONS, to _LIBUNWIND_HIDE_SYMBOLS,
because now the macro adds a .hidden directive rather than merely
suppress visibility annotations.

For ELF, when LIBUNWIND_HIDE_SYMBOLS is enabled, mark unw_getcontext as
hidden. This symbol is the only one defined using src/assembly.h's
WEAK_ALIAS macro. Other unw_* weak aliases are defined in C++ and are
already hidden.

Mach-O doesn't support weak aliases, so remove .weak_reference and
weak_import. When LIBUNWIND_HIDE_SYMBOLS is enabled, output
.private_extern for the unw_* aliases.

In assembly.h, add missing SYMBOL_NAME macro invocations, which are
used to prefix symbol names with '_' on some targets.

Fixes PR46709.

Diff Detail

Event Timeline

rprichard created this revision.Dec 9 2020, 11:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2020, 11:14 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
rprichard requested review of this revision.Dec 9 2020, 11:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2020, 11:14 PM

Fix LIBUNWIND_HERMETIC_STATIC_LIBRARY on macOS.

rprichard retitled this revision from [libunwind] Fix ELF/Mach-O visibility for unw_getcontext to [libunwind] LIBUNWIND_HERMETIC_STATIC_LIBRARY fixes.Dec 10 2020, 12:38 AM
rprichard edited the summary of this revision. (Show Details)

I assume the LIBUNWIND_HERMETIC_STATIC_LIBRARY setting isn't used on Darwin -- it leaves unw_* symbols as Extern and not PrivateExtern. Maybe the CMake file could reject that flag on Darwin instead of trying to make it work.

AFAICT, the existing unw_* aliases aren't weak on macOS. For aliases to C++ functions, the __attribute__((weak_import)) didn't seem to do anything when I tested it. I think the declared unw_* functions aren't referenced in the TU that declares them, so the compiler doesn't do anything with the declaration. When I experimented, if I *did* reference the function, then the compiler outputted a .weak_reference directive. However, AFAIK, the assembler ignores the .weak_reference directive and uses the weakdef/weakref flags from the target of the alias instead (i.e. the __unw_* function).

For unw_getcontext specifically, assembly.h's WEAK_SYMBOL(aliasname) SEPARATOR omitted the SYMBOL_NAME macro on Darwin, so I think the .weak_reference would have referred to the wrong symbol. For arm64 iphoneos, I saw this preprocessed output:

.globl _unw_getcontext %% .weak_reference unw_getcontext %% _unw_getcontext = ___unw_getcontext

Adding SYMBOL_NAME doesn't make unw_getcontext weak.

I'm not sure if the aliases can actually be marked weak on Darwin, so instead, I removed the code making them weak. Windows already leaves the symbols non-weak. I'm not very familiar with Mach-O, though, so maybe I'm wrong here. In particular, maybe there's some configuration where it works (e.g. where the assembler behaves differently).

libunwind/CMakeLists.txt
325

This code block effectively duplicates the LIBUNWIND_HERMETIC_STATIC_LIBRARY block in libunwind/src/CMakeLists.txt, assuming that the -fvisibility flags are ignored for Windows. It's on by default, though, whereas the LIBUNWIND_HERMETIC_STATIC_LIBRARY option is off by default. Maybe this code block should be removed, but I suppose that would break current users, quietly.

libunwind/src/CMakeLists.txt
167

e.g. Maybe this could be: if(LIBUNWIND_HERMETIC_STATIC_LIBRARY OR WIN32)

Or maybe we can default LIBUNWIND_HERMETIC_STATIC_LIBRARY to ON for WIN32? I'm not sure what's possible with CMake.

169

I think this -fvisibility-global-new-delete-hidden flag has no effect, but if it did have an effect, it could break the NDK's libc++_shared.so when it's linked with libunwind.a.

i.e. AFAIK, libunwind doesn't define or use the global new/delete operators. (D57251 switched to placement new.) If libunwind merely referenced new/delete, then this flag would make the undefined symbol reference hidden, and then linking libunwind.a into the NDK's libc++_shared.so could make the new/delete definitions hidden (because undef-hidden + def-default ==> def-hidden).

It was added with D57107.

compnerd requested changes to this revision.Jan 13 2021, 8:44 AM
compnerd added a subscriber: compnerd.
compnerd added inline comments.
libunwind/CMakeLists.txt
324

Similar to the argument for the naming for the compiler-rt builtins, would you mind renaming this to _LIBUNWIND_HIDE_SYMBOLS please?

325

IIRC, -fvisibility= is not ignored and is an error. Either way, -fvisibility= is meaningless on Windows - you *must* control the behaviour via macros and __declspec(dllexport) for the DLL build, and nothing for the static build (which makes it internal).

libunwind/src/CMakeLists.txt
167

-fvisibility= should not be set for Win32. This is correct as is.

libunwind/src/assembly.h
81

Does this not change the behaviour on MachO? This symbol is now private_extern rather than a weak_reference. A weak reference will be set to 0 by the loader if it is not found, and a private_extern is a strong internal reference.

103

I don't understand how this works. This is making the symbol a forward declaration, which is not a weak symbol definition.

This revision now requires changes to proceed.Jan 13 2021, 8:44 AM
rprichard added inline comments.Jan 16 2021, 12:38 AM
libunwind/CMakeLists.txt
324

Should I also rename the CMake variable? And if so, do I need to continue recognizing the old name for backwards compatibility?

If we change the CMake variable name, at least ./clang/cmake/caches/Fuchsia-stage2.cmake will need to be updated.

libunwind/src/assembly.h
81

Is .weak_reference the right directive to use here, instead of .weak_definition? We're defining a symbol (aliasname) and setting its value to that of another symbol (name).

I think marking unw_* weak is intended to let some other strong definition override it. Its value won't ever be set to 0.

Currently on Mach-O, the hide-symbols flag hides almost everything (including _Unwind_*) but leaves all of the unw_* alias symbols as extern (and not private-extern) and not weak. With my change, they're still not weak, but they're private-extern.

libunwind's current assembly.h behavior for a weak alias:

.globl aliasname
.weak_reference aliasname
aliasname = name

The LLVM Mach-O assembler ignores the .weak_reference directive. If I change it to .weak_definition, it is still ignored. AFAICT, the LLVM assembler uses the WeakDef/WeakRef attributes from the name symbol.

e.g.

$ cat test.S    
    .text
    .space 0x42

    // Define foo.
    .globl foo
foo:
    ret

    // Define a weak alias, bar.
    .globl bar
    .weak_reference bar
    bar = foo

$ ~/clang11/bin/clang test.S -c && ~/clang11/bin/llvm-readobj --syms test.o

File: test.o
Format: Mach-O 64-bit x86-64
Arch: x86_64
AddressSize: 64bit
Symbols [
  Symbol {
    Name: bar (1)
    Extern
    Type: Section (0xE)
    Section: __text (0x1)
    RefType: UndefinedNonLazy (0x0)
    Flags [ (0x0)
    ]
    Value: 0x42
  }
  Symbol {
    Name: foo (5)
    Extern
    Type: Section (0xE)
    Section: __text (0x1)
    RefType: UndefinedNonLazy (0x0)
    Flags [ (0x0)
    ]
    Value: 0x42
  }
]

The Flags are empty.

OTOH, if I flip things around:

    .text
    .space 0x42

    // Define a weak function, foo.
    .globl foo
    .weak_reference foo
foo:
    ret

    // Define an alias, bar.
    .globl bar
    bar = foo

Now both symbols are WeakRef:

File: test.o
Format: Mach-O 64-bit x86-64
Arch: x86_64
AddressSize: 64bit
Symbols [
  Symbol {
    Name: bar (1)
    Extern
    Type: Section (0xE)
    Section: __text (0x1)
    RefType: UndefinedNonLazy (0x0)
    Flags [ (0x40)
      WeakRef (0x40)
    ]
    Value: 0x42
  }
  Symbol {
    Name: foo (5)
    Extern
    Type: Section (0xE)
    Section: __text (0x1)
    RefType: UndefinedNonLazy (0x0)
    Flags [ (0x40)
      WeakRef (0x40)
    ]
    Value: 0x42
  }
]

I'm wondering if this LLVM behavior is actually correct, but I'm also unfamiliar with Mach-O. (i.e. We want to copy the symbol's address, but should we be copying the WeakRef/WeakDef flags?) It looks like the behavior is controlled in MachObjectWriter::writeNlist. writeNlist finds the aliased symbol and uses its flags:

// The Mach-O streamer uses the lowest 16-bits of the flags for the 'desc'
// value.
bool EncodeAsAltEntry =
  IsAlias && cast<MCSymbolMachO>(OrigSymbol).isAltEntry();
W.write<uint16_t>(cast<MCSymbolMachO>(Symbol)->getEncodedFlags(EncodeAsAltEntry));

The PrivateExtern attribute, on the other hand, isn't part of these encoded flags:

if (Data.isPrivateExtern())
  Type |= MachO::N_PEXT;

Data continues to refer to the alias symbol rather than the aliased symbol. However, apparently the author isn't completely sure about things?

// FIXME: Should this update Data as well?

In libunwind, there is one usage of assembly.h's WEAK_ALIAS. UnwindRegistersSave.S defines a hidden non-weak __unw_getcontext function, and also a weak alias unw_getcontext. My patch's goal is to make unw_getcontext hidden or not-hidden depending on a CMake config variable.

Currently, on Mach-O and on Windows, WEAK_ALIAS doesn't actually make the alias weak. I'm just making it a bit more explicit on Mach-O.

Alternatively:

  • Instead of a weak alias, we could output a weak thunk -- a weak function with a branch to the internal hidden symbol. That's more of a functional change, though.
  • Or, on Mach-O, both the unw_* and __unw_* functions could be WeakDef.
  • Maybe the hide-symbols flag should only affect ELF?
103

Currently, there's only one use of WEAK_ALIAS, at the end of UnwindRegistersSave.S, which defines unw_getcontext to have the same value as __unw_getcontext, which was defined earlier in the file. It's generating one of:

.hidden unw_getcontext
.weak unw_getcontext
unw_getcontext = __unw_getcontext

or:

.weak unw_getcontext
unw_getcontext = __unw_getcontext
rprichard updated this revision to Diff 321701.Feb 5 2021, 4:02 AM

Just fix ELF and leave Mach-O as-is.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2021, 4:02 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

After renaming the CMake option, CMake prints a warning if I set the old option:

CMake Warning:
  Manually-specified variables were not used by the project:

    LIBUNWIND_HERMETIC_STATIC_LIBRARY
rprichard retitled this revision from [libunwind] LIBUNWIND_HERMETIC_STATIC_LIBRARY fixes to [libunwind][ELF] Hide unw_getcontext with LIBUNWIND_HIDE_SYMBOLS.Feb 5 2021, 4:04 AM
rprichard edited the summary of this revision. (Show Details)
steven_wu added inline comments.
libunwind/src/assembly.h
81

I guess the symbol is never really weak for Darwin. The weak_import attribute will turn all the reference to the symbol to weak_reference but since the alias is declared in .cpp file and never referenced, it will not create any weak linkage to the symbol. I am also not sure if a weak alias is possible on Darwin :) I think making everything .private_extern for Darwin should be fine?

@ldionne The change was added: https://reviews.llvm.org/D59921. I am not sure why the alias need to be weak?

rprichard added inline comments.Feb 8 2021, 11:24 PM
libunwind/src/assembly.h
81

I guess the symbol is never really weak for Darwin. The weak_import attribute will turn all the reference to the symbol to weak_reference but since the alias is declared in .cpp file and never referenced, it will not create any weak linkage to the symbol. I am also not sure if a weak alias is possible on Darwin :) I think making everything .private_extern for Darwin should be fine?

@ldionne The change was added: https://reviews.llvm.org/D59921. I am not sure why the alias need to be weak?

I'm guessing the unw_* symbols are marked weak because they're not "reserved for the implementation" the way the underscore-uppercase symbols are (_Unwind_*), so if a program wants to define its own unw_* symbols, it should be able to without breaking unwinding. This situation could cause a multiply-defined symbol error at link-time, at least when libunwind is linked statically.

rprichard updated this revision to Diff 322574.Feb 9 2021, 7:49 PM
rprichard edited the summary of this revision. (Show Details)

Restore Mach-O alias fixes.

rprichard added inline comments.Feb 9 2021, 7:57 PM
libunwind/src/assembly.h
81

Ok, I've restored the Mach-O part of this change. Please take a look.

I did confirm that weak aliases don't work on Mach-O: when a weak function has been overridden with a strong function, the linker can delete the weak function, leaving the aliases pointing at the deleted area (i.e. whatever function happens to come next). https://gist.github.com/rprichard/8775d7844228577e40d2fd0776397f47

I also removed the WEAK_SYMBOL() macro for the Apple case, because it's not used, Windows also doesn't have the macro, and it's not clear whether .weak_definition or .weak_reference would be wanted.

emaste: Just a heads up in case FreeBSD is affected. FWIW, I noticed that libgcc_eh.a on FreeBSD 12.1 doesn't hide the _Unwind_* or unw_* symbols. The build system is defining VISIBILITY_HIDDEN but libunwind doesn't respond to that name. FreeBSD libgcc_eh.a also defines hidden symbols named logAPIs, logUnwinding, and logDWARF. libunwind defines these internal names if NDEBUG isn't defined. They can break statically-linked programs on FreeBSD (e.g. duplicate symbol linker errors).

rprichard retitled this revision from [libunwind][ELF] Hide unw_getcontext with LIBUNWIND_HIDE_SYMBOLS to [libunwind] unw_* alias fixes for ELF and Mach-O.Feb 10 2021, 12:56 AM
rprichard edited the summary of this revision. (Show Details)
rprichard updated this revision to Diff 323221.Feb 11 2021, 9:49 PM
rprichard edited the summary of this revision. (Show Details)

Update libunwind/src/BUILD.gn for macro name change.

rprichard updated this revision to Diff 323222.Feb 11 2021, 9:52 PM

Rebase and fix merge conflict in gn file.

Maybe this is blocked on someone from Apple reviewing the Mach-O parts?

Maybe this is blocked on someone from Apple reviewing the Mach-O parts?

This is fine with me for how Apple builds libunwind. I am not sure if the open source libunwind build for MachO do expect different behavior but I don't think there are other reasonable behaviors.

I can give my LGTM if @ldionne has no problem with that.

Maybe this is blocked on someone from Apple reviewing the Mach-O parts?

This is fine with me for how Apple builds libunwind. I am not sure if the open source libunwind build for MachO do expect different behavior but I don't think there are other reasonable behaviors.

I can give my LGTM if @ldionne has no problem with that.

Oh, I had missed that. I'm not a libunwind owner, but if Steven says it's good for Apple, then it is. I trust him a lot more than I trust myself in this area.

It sounds like everyone is happy here but the tools. Could we get a libunwind reviewer (preferably @compnerd, since his review is the red one) to LGTM this?

steven_wu accepted this revision.Feb 19 2021, 5:26 PM

My main concern is just that weak alias for libc++abi that never worked for Darwin :)

LGTM.

phosek accepted this revision.Feb 19 2021, 9:21 PM

LGTM

I'd also suggest renaming LIBCXXABI_HERMETIC_STATIC_LIBRARY and LIBCXX_HERMETIC_STATIC_LIBRARY to LIBCXXABI_HIDE_SYMBOLS and LIBCXX_HIDE_SYMBOLS respectively in a follow up change to maintain consistency.

compnerd accepted this revision.Feb 22 2021, 2:22 PM
This revision is now accepted and ready to land.Feb 22 2021, 2:22 PM
This revision was landed with ongoing or failed builds.Feb 22 2021, 4:54 PM
This revision was automatically updated to reflect the committed changes.