steven_wu added inline comments.
================
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)
----------------
rprichard wrote:
> 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?
>
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93003/new/
https://reviews.llvm.org/D93003
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
