[libcxx-commits] [PATCH] D93003: [libunwind] LIBUNWIND_HERMETIC_STATIC_LIBRARY fixes

Sat Jan 16 00:38:47 PST 2021

rprichard added inline comments.

================
Comment at: libunwind/CMakeLists.txt:332
 if (WIN32 AND LIBUNWIND_ENABLE_STATIC AND NOT LIBUNWIND_ENABLE_SHARED)
-  add_definitions(-D_LIBUNWIND_DISABLE_VISIBILITY_ANNOTATIONS)
+  add_definitions(-D_LIBUNWIND_HERMETIC_STATIC_LIBRARY)
 endif()
----------------
compnerd wrote:
> Similar to the argument for the naming for the compiler-rt builtins, would you mind renaming this to `_LIBUNWIND_HIDE_SYMBOLS` please?
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.

================
Comment at: libunwind/src/assembly.h:82
   .globl SYMBOL_NAME(aliasname) SEPARATOR                                      \
-  WEAK_SYMBOL(aliasname) SEPARATOR                                             \
+  EXPORT_SYMBOL(SYMBOL_NAME(aliasname)) SEPARATOR                              \
   SYMBOL_NAME(aliasname) = SYMBOL_NAME(name)
----------------
compnerd wrote:
> 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.
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?

================
Comment at: libunwind/src/assembly.h:104
+#define WEAK_ALIAS(name, aliasname)                                            \
+  EXPORT_SYMBOL(SYMBOL_NAME(aliasname)) SEPARATOR                              \
+  WEAK_SYMBOL(SYMBOL_NAME(aliasname)) SEPARATOR                                \
----------------
compnerd wrote:
> I don't understand how this works.  This is making the symbol a forward declaration, which is not a weak symbol definition.
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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93003/new/

https://reviews.llvm.org/D93003