Page MenuHomePhabricator

[libcxx] Support generating linker script for static library
Needs ReviewPublic

Authored by phosek on Nov 19 2018, 2:14 PM.

Details

Reviewers
EricWF
ldionne
Group Reviewers
Restricted Project
Summary

When supported, we also generate linker script for static library,
renaming it to libc++static.a. The linker scripts also includes any
additional dependencies that the static library has.

Diff Detail

Repository
rCXX libc++

Event Timeline

phosek created this revision.Nov 19 2018, 2:14 PM
ldionne requested changes to this revision.Nov 19 2018, 3:24 PM
ldionne added inline comments.
libcxx/lib/CMakeLists.txt
381

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?

385

Why call it libc++static.a instead of libc++.a?

388

Is that true on all platforms? On what platforms is this linker script expected to work?

libcxx/utils/gen_link_script.py
11

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?

25

I think a word is missing somewhere because this help seems unclear to me.

29

List of libraries? Just any libraries?

This revision now requires changes to proceed.Nov 19 2018, 3:24 PM
phosek added inline comments.Nov 19 2018, 3:46 PM
libcxx/lib/CMakeLists.txt
385

We rename libc++.a to libc++static.a and then create linker script called libc++.a in its place that contains INPUT(libc++static.a ...). Like I said in my email, I'm not a big fan of that name so I welcome other suggestions.

ChromeOS toolchain already does this in their toolchain build and they use the name libc++_static.a (I dropped _ because we already have libc++fs.a and libc++experimental.a), libstdc++ uses the same approach and they use the name libstdc++_nonshared.a.

phosek added inline comments.Nov 19 2018, 3:48 PM
libcxx/utils/gen_link_script.py
11

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?

ldionne added inline comments.Nov 19 2018, 4:04 PM
libcxx/utils/gen_link_script.py
11

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?

22–42

Please add general description of what the script does, like there used to be.

27

Please consider making these named parameters instead of positional parameters. That would have answered some of my questions before I had to ask them.

phosek updated this revision to Diff 178948.Dec 19 2018, 1:14 PM
phosek marked 5 inline comments as done.
phosek added inline comments.
libcxx/lib/CMakeLists.txt
381

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.

libcxx/utils/gen_link_script.py
11

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?

27

I find it little awkward to use optional arguments as required arguments but I can do that if you prefer?

ldionne added inline comments.Dec 20 2018, 9:01 AM
libcxx/lib/CMakeLists.txt
388

I liked c++static more than c++nonshared, personally.

libcxx/utils/gen_link_script.py
11

Ok, thanks for explaining.

27

You can have required named arguments: parser.add_argument('--foo', required=True). https://docs.python.org/2.7/library/argparse.html#required

phosek updated this revision to Diff 179189.Dec 20 2018, 3:51 PM
phosek marked an inline comment as done and an inline comment as not done.
phosek added inline comments.Dec 20 2018, 5:02 PM
libcxx/utils/gen_link_script.py
27

I know, I was just pointing out that I find it a little awkward to use "optional" arguments as required arguments which is what "positional" is typically used for, but that's just my personal preference. I'm fine changing that if you prefer.

ldionne accepted this revision.Dec 21 2018, 2:18 PM

Ship it, with named parameters. Don't wait over the holidays.

libcxx/utils/gen_link_script.py
27

I don't understand why you say they're optional arguments.. To me, they are "named" arguments. Optional is when required=False, mandatory is when required=True. But regardless, I do think that using named arguments will be much better because of the self documentation -- I don't want to have to read this script to figure out what the command in the CMake is doing.

This revision is now accepted and ready to land.Dec 21 2018, 2:18 PM
ldionne added a reviewer: Restricted Project.Mon, Nov 2, 3:04 PM

@phosek do you still want to do this?

This revision now requires review to proceed.Mon, Nov 2, 3:04 PM