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

[Louis Dionne via Phabricator]

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
+    COMMAND
----------------
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
+      "${CMAKE_STATIC_LIBRARY_PREFIX}c++static${CMAKE_STATIC_LIBRARY_SUFFIX}"
+      "$<TARGET_LINKER_FILE:cxx_static>"
----------------
Why call it `libc++static.a` instead of `libc++.a`?


================
Comment at: libcxx/lib/CMakeLists.txt:395
+      ${LIBCXX_INTERFACE_LIBRARY_NAMES}
+      "m" "dl" "pthread"
+    BYPRODUCTS
----------------
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?


Repository:
  rCXX libc++

https://reviews.llvm.org/D54726