[PATCH] D54726: [libcxx] Support generating linker script for static library

Louis Dionne via Phabricator reviews at reviews.llvm.org
Mon Nov 19 15:25:04 PST 2018

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

Comment at: libcxx/lib/CMakeLists.txt:388
+  add_custom_command(TARGET cxx_static POST_BUILD
There's something I don't understand here. You seem to be calling the script as `<script> output input` (in that order) here, but the script seems to take `input output` based on the `argparse` stuff below. If `"${CMAKE_STATIC_LIBRARY_PREFIX}c++static${CMAKE_STATIC_LIBRARY_SUFFIX}"` is the input file, where is it created?

Comment at: libcxx/lib/CMakeLists.txt:392
+    ARGS
+      "$<TARGET_LINKER_FILE:cxx_static>"
Why call it `libc++static.a` instead of `libc++.a`?

Comment at: libcxx/lib/CMakeLists.txt:395
+      "m" "dl" "pthread"
Is that true on all platforms? On what platforms is this linker script expected to work?

Comment at: libcxx/utils/gen_link_script.py:11
+import argparse
 import os
Can you confirm that there are no functional changes beyond adding support for a custom output file in this script? This is only a refactoring (thanks!), or did I miss something?

Comment at: libcxx/utils/gen_link_script.py:20
+                        action="store_true", default=False)
+    parser.add_argument("--rename", help="Rename output file if exists",
+                        action="store_true", default=False)
I think a word is missing somewhere because this help seems unclear to me.

Comment at: libcxx/utils/gen_link_script.py:24
+    parser.add_argument("output", help="Path to libc++ linker script")
+    parser.add_argument("libraries", help="List of libraries", nargs="*")
+    args = parser.parse_args()
List of libraries? Just any libraries?

  rCXX libc++


More information about the libcxx-commits mailing list