[PATCH] D60370: [gn] Support for building libunwind

[Petr Hosek via Phabricator via llvm-commits]

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


================
Comment at: llvm/utils/gn/secondary/libunwind/src/BUILD.gn:23
+
+unwind_sources = [
+  "libunwind.cpp",
----------------
thakis wrote:
> Does making this a source_set and having both libs depend on that not work for static_library?
I did that originally, but I changed it to build static and shared library separately, which follows the recent change to CMake build which also stopped using object library: https://github.com/llvm/llvm-project/commit/1de15f6f336dba6e26c18c224d1b53ba617f57f0#diff-2467f5cba1ecc34d53f4480cd654aa55 which is itself is a follow up to http://llvm.org/viewvc/llvm-project?view=revision&revision=356150.

The reason why we did that for libunwind, libc++abi and libunwind is because it's sometimes necessary to use different options for the static and shared library, e.g. on some platforms static library would be built without `-fPIC` but shared would still use it. In case of Fuchsia we also use `libunwind_hermetic_static_library` option which requires building static library objects with `-fvisibility=hidden`. This required a lot of complicated logic, not using the source set actually makes it simpler in this case.


================
Comment at: llvm/utils/gn/secondary/libunwind/src/BUILD.gn:51
+  cflags_c = [ "-std=c99" ]
+  cflags_cc = [ "-fno-rtti" ]
+  include_dirs = [ "//libunwind/include" ]
----------------
thakis wrote:
> why do you remove config_nortti and then add -fno-rtti back in here? Can't you just not remove it in the first place?
In CMake build, it's possible to re-enable RTTI using the `LLVM_ENABLE_RTTI` that some users actually use, especially when linking LLVM into other projects. Do we plan on providing the same flag in LLVM build? If yes, that would be an argument for setting `-fno-rtti` here explicitly because it should be independent of the global LLVM setup.


================
Comment at: llvm/utils/gn/secondary/libunwind/src/BUILD.gn:77
+    sources = unwind_sources
+    # TODO: sync_source_lists_from_cmake.py doesn't handle these.
+    #public = unwind_headers
----------------
thakis wrote:
> It looks for .h files, no matter if in a target or not. Does not adding this to public help the script? That'd surprise me.
I think the problem is that the CMake build uses `${CMAKE_CURRENT_SOURCE_DIR}/../include/libunwind.h` while I tried using `../include/libunwind.h`, the script doesn't seem to handle variables or `..`. Any suggestions how to work around it?


Repository:
  rUNW libunwind

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

https://reviews.llvm.org/D60370