[PATCH] D93003: [libunwind] LIBUNWIND_HERMETIC_STATIC_LIBRARY fixes

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd requested changes to this revision.
compnerd added inline comments.
This revision now requires changes to proceed.


================
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()
----------------
Similar to the argument for the naming for the compiler-rt builtins, would you mind renaming this to `_LIBUNWIND_HIDE_SYMBOLS` please?


================
Comment at: libunwind/CMakeLists.txt:333
+  add_definitions(-D_LIBUNWIND_HERMETIC_STATIC_LIBRARY)
 endif()
 
----------------
rprichard wrote:
> 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.
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).


================
Comment at: libunwind/src/CMakeLists.txt:163
 
   if(LIBUNWIND_HERMETIC_STATIC_LIBRARY)
     append_flags_if_supported(UNWIND_STATIC_LIBRARY_FLAGS -fvisibility=hidden)
----------------
rprichard wrote:
> 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.
`-fvisibility=` should not be set for Win32.  This is correct as is.


================
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)
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93003