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

[Petr Hosek via Phabricator]

phosek added inline comments.


================
Comment at: libcxx/lib/CMakeLists.txt:388
   )
+  add_custom_command(TARGET cxx_static POST_BUILD
+    COMMAND
----------------
ldionne wrote:
> 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?
`input` is `TARGET_SONAME_FILE:cxx_shared` which would be `libc++.so.1`, `output` is `TARGET_LINKER_FILE:cxx_shared` which would be `libc++.so`. With these arguments `gen_link_script.py` generates `libc++.so` linker script that has `libc++.so.1` as input, which matches the current behavior.


================
Comment at: libcxx/utils/gen_link_script.py:11
 
+import argparse
 import os
----------------
ldionne wrote:
> phosek wrote:
> > ldionne wrote:
> > > 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?
> > The original script did not support creating a linker script for static library, so there's a functional change, but I also refactored the code a bit to make it (subjectively) simpler. I'd be fine doing the refactoring as a separate change if you prefer?
> No, I'm fine with the refactoring + the functional change, I was just trying to spot what the functional change was. I'm sorry, but I still don't see what prevented the previous script from handling static archives. Can you please point it out?
Previous version assumes that the input file (libc++) is a symlink which would be the case for `libc++.so` which a symlink to `libc++.so.1`. The script replace that symlink with a linker script that includes the library that the symlink was originally pointing to.

However, this doesn't work for static library which is a plain file `libc++.a`. The new version now handles input files that are not symlinks and it also allows renaming the input to output (the `--rename` option), so `libc++.a` would be first renamed to `libc++static.a` and generated linker script would include it as input.

Does this make sense?


================
Comment at: libcxx/utils/gen_link_script.py:22
+                        action="store_true", default=False)
+    parser.add_argument("input", help="Path to libc++ library")
+    parser.add_argument("output", help="Path to libc++ linker script")
----------------
ldionne wrote:
> Please consider making these named parameters instead of positional parameters. That would have answered some of my questions before I had to ask them.
I find it little awkward to use optional arguments as required arguments but I can do that if you prefer?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D54726