[libcxx-commits] [PATCH] D59921: [libunwind] Export the unw_* symbols as weak symbols

[Petr Hosek via Phabricator via libcxx-commits]

phosek marked 2 inline comments as done.
phosek added inline comments.


================
Comment at: libunwind/src/assembly.h:95
+  WEAK_SYMBOL(aliasname)                                                       \
+  SEPARATOR alias<SYMBOL_NAME(aliasname)> = <SYMBOL_NAME(name)>
 
----------------
mstorsjo wrote:
> What assembler syntax is this `alias<somesymbol> = <othersymbol>`? The LLVM assembler certainly doesn't support it at least. Is it some thing for a microsoft assembler? The rest of this code doesn't assemble with any such tool anyway. Or is it a leftover marker indicating that you meant to do something?
> 
> If we go with `/alternatename` via a pragma on the C level, the corresponding thing here would be an alternatename directive in the exact same way as `EXPORT_SYMBOL2` above. If we go with `__attribute__((weak,alias()))`, or `__attribute__((alias()))` the corresponding thing is the same as for ELF above, with or without the `.weak` directive.
It's the [masm syntax](https://docs.microsoft.com/en-us/cpp/assembler/masm/alias-masm?view=vs-2017), but I'll replace it with the alternative as you suggested.


================
Comment at: libunwind/src/config.h:77
+#define _LIBUNWIND_WEAK_ALIAS(name, aliasname)                                 \
+  __pragma(comment(linker, "/alternatename:" #aliasname "=" #name))            \
+      _LIBUNWIND_EXPORT extern "C" __typeof(name) aliasname;
----------------
mstorsjo wrote:
> rnk wrote:
> > mstorsjo wrote:
> > > This kind of pragma requires building with `-fms-extensions` for mingw mode (which is the only windows configuration where there's any need to use libunwind at all, as far as I know), which breaks building with gcc (and the directive is only supported by lld, not by ld.bfd). This requires adding `unwind_append_if(LIBUNWIND_COMPILE_FLAGS MINGW -fms-extensions)` to the main `CMakeLists.txt`.
> > > 
> > > That's not a blocker for this change by any means, but I'll just let it be noted that it's a change from how things were before.
> > > 
> > > The `__attribute__((weak, alias()))` form does work in both gcc and clang actually though, so that might almost also be an option, but it's a bit messy (both clang and ld.bfd seem to have a bit of gotchas relating to it), so perhaps we shouldn't.
> > > 
> > > Alternatively, we could also just postpone making them properly weak on mingw (as it's not strictly necessary contrary to e.g. sanitizers), and just do this with a plain `__attribute__((alias(#name)))` as above, without the weak.
> > > 
> > > Also, I'm pretty sure that `/alternatename` requires using `__USER_LABEL_PREFIX__` here (judging from how it's used in sanitizers).
> > Re: `__USER_LABEL_PREFIX__`, I think that's true. It's a pretty big drawback to /alternatename.
> > 
> > I guess @phosek wants to make llvm libunwind consistently not provide strong definitions of unw_* across all platforms, otherwise I would say just make this whole thing ELF-only, and provide strong aliases of unw_* = _unw_* for non-ELF platforms.
> > 
> > Does libunwind build with MSVC on Windows? I have no idea, personally. I feel like it's only ever brought in if Itanium-style EH is in use, which usually means mingw, or @compnerd's emergent *-windows-itanium ABI, so theoretically, it might build with MSVC.
> I don't think there's anything particularly tricky at all on the C level so far, so I would expect it to build with MSVC just fine. But the assembly files require a gas style assembler.
We're only concerned with ELF for our case, but I'd prefer to keep all platforms as similar as possible unless there's technical reason why it cannot be done. It seems like in this case it's still feasible to use the same approach for ELF, COFF and Mach-O.


Repository:
  rUNW libunwind

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

https://reviews.llvm.org/D59921