introducing llvm-libtool-darwin

[sameerarora101]

Summary

This diff starts the implementation of llvm-libtool-darwin
(an llvm based replacement of cctool's libtool for darwin format).

Libtool is used for creating static and dynamic libraries
from a bunch of object files given as input.

In this diff, I implement the --help and --version options. I
also add support for Input and Output files so as to print the
appropriate help message.

[sameerarora101]

Resolving clang-tidy and test failure on windows

[jhenderson]

1. Has the introduction of a new tool been discussed on llvm-dev? (I vaguely recall that it might have been but might be getting confused). Please include a link to the mailing thread in the description.

2. Is this significantly different to llvm-ar and its aliases? If so, please outline the differences.

3. Assuming this is going to be developed, could you split off one or two patches that make this even simpler, please. For example, the very first patch would just add the tool with only the built-in --help/--version options, not doing anything useful.

4. You may want to consider using cl::opt instead of the tablegen approach you're currently using, since it's simpler and I doubt you need complex logic here.

5. There are some basic error handling functions available in the Support library which might do all you need them to. Consider using them before rolling your own.

5. Is there going to be non-Darwin versions of this tool? If not, it doesn't need to be located in the Darwin sub-directory.

[sameerarora101]

1. Has the introduction of a new tool been discussed on llvm-dev? (I vaguely recall that it might have been but might be getting confused). Please include a link to the mailing thread in the description.

Yup, here is the link for the thread : http://lists.llvm.org/pipermail/llvm-dev/2020-June/142157.html

2. Is this significantly different to llvm-ar and its aliases? If so, please outline the differences.

No, it is not significantly different from llvm-ar but has good defaults for Apple platforms (e.g. automatically creating a table of contents with the right format). To get the same behavior from llvm-ar, we have to explicitly pass additional flags. Furthermore, for archive input files, the individual objects in the input archive are added to the output archive by default by libtool. This too requires additional flags in the case of llvm-ar.

3. Assuming this is going to be developed, could you split off one or two patches that make this even simpler, please. For example, the very first patch would just add the tool with only the built-in --help/--version options, not doing anything useful.
4. You may want to consider using cl::opt instead of the tablegen approach you're currently using, since it's simpler and I doubt you need complex logic here.
5. There are some basic error handling functions available in the Support library which might do all you need them to. Consider using them before rolling your own.

Ok, got it. Thanks

6. Is there going to be non-Darwin versions of this tool? If not, it doesn't need to be located in the Darwin sub-directory.

Quoting from the mailing thread here (http://lists.llvm.org/pipermail/llvm-dev/2020-June/142284.html) :

An alternative is to plan for the tool to eventually grow to support the features of GNU libtool, allowing a contributor who may become interested in that to work on it within the same code base.

and

We could take the approach used by LLD and llvm-objcopy, where we create a directory for the tool (llvm/tools/llvm-libtool in this case), and have subdirectories for the ports of the tool (Darwin in our case, and someone interested could create a GNU subdirectory later). That way there's a natural place to put any code that's common between the ports.

@smeenai Please correct me if I highlighted anything wrong above. Thanks!

[smeenai]

@sameerarora101 answered a bunch of this above, but to add on a bit.

In D82923#2124779, @jhenderson wrote:

2. Is this significantly different to llvm-ar and its aliases? If so, please outline the differences.

http://lists.llvm.org/pipermail/llvm-dev/2020-June/142157.html highlights why we're creating this as a separate tool instead of an llvm-ar frontend, and the motivating features for that (e.g. universal binary support) should become clearer in follow-up diffs.

4. You may want to consider using cl::opt instead of the tablegen approach you're currently using, since it's simpler and I doubt you need complex logic here.

5. There are some basic error handling functions available in the Support library which might do all you need them to. Consider using them before rolling your own.

Both of these are the result of using llvm-lipo as a reference, but thinking back, llvm-lipo started with cl::opt and then switched to tablegen to handle more complicated argument parsing logic. Looking at libtool, I think cl::opt should be enough for its argument parsing needs, though I'd wanna think about that a bit more.

I thought there was also a mailing list discussion a while back about the two option parsing libraries and which one to prefer for new tools, though I can't remember the conclusions of that discussion (or even find it now).

For reference, cl::opt is documented at https://llvm.org/docs/CommandLine.html.

5. Is there going to be non-Darwin versions of this tool? If not, it doesn't need to be located in the Darwin sub-directory.

I'd want to keep the llvm-libtool-darwin name, to prevent confusion with GNU libtool (which is an entirely different tool), but I'm happy not having the sub-directory until someone actually intends to contribute a GNU version. (We have no such plans.)

[alexander-shaposhnikov]

I didn't reply on the mailing list, but to me the motivation for this tool sounds reasonable too.
I'll try to take a look at the code a bit later this week.

[smeenai]

Looking at libtool's options further, the only concern I have is with:

-      Treat all remaining arguments as names of files (or archives) and not as options.

In other words, if you include a single - by itself on your command line, all remaining arguments are parsed as input names. cl::opt supports similar functionality, but using -- instead of -. I don't know if the TableGen parsing can support that either though, so I don't know if it would be possible for us to keep the exact same command line interface (with respect to this flag) anyway.

[jyknight]

I suggest removing the extraneous level of directory "Darwin" -- just put the files in "llvm/tools/llvm-libtool-darwin", name the source "llvm-libtool-darwin.cpp", and say you're adding "llvm-libtool-darwin" in the commit message.

While it's weird to have a darwin-specific tool for creating archives and shared libraries, that's the status quo. And moving that to an LLVM implementation is entirely reasonable. Let's just not pretend this is ever going to be something else other than that.

[tschuett]

In D82923#2126259, @jyknight wrote:

I suggest removing the extraneous level of directory "Darwin" -- just put the files in "llvm/tools/llvm-libtool-darwin", name the source "llvm-libtool-darwin.cpp", and say you're adding "llvm-libtool-darwin" in the commit message.

While it's weird to have a darwin-specific tool for creating archives and shared libraries, that's the status quo. And moving that to an LLVM implementation is entirely reasonable. Let's just not pretend this is ever going to be something else other than that.

Then you can name it llvm-libtool without any sub-directories and revisit the situation once somebody wants to the GNU thing, which is never going to happen.

[sameerarora101]

updates:

• currently storing the tool under tools/llvm-libtool-darwin. I can update this again if people prefer something else.
• parsing arguments using cl::opt
• adding support only for --help and --version options.
• had to add argument parsing for input/output files to print the appropriate help message

[MaskRay]

llvm-libtool-darwin is a good name to me. GNU Libtool is a very different project. I doubt we will add an LLVM clone... libtool is a shell script with 10000+ lines of code:)

[MaskRay]

llvm/test/tools/llvm-libtool-darwin/help-message.test

5

Does cctool libtool use -long-option as the canonical spelling?

For comparison, GNU getopt convention is --long-option

llvm/tools/llvm-libtool-darwin/LLVMBuild.txt

20

Have you tried a -DBUILD_SHARED_LIBS=on cmake build and ensured ninja llvm-libtool-darwin can build? -DBUILD_SHARED_LIBS=on sometimes has stricter dependency requirement. Every direct dependency must be listed to make -z defs happy.

[jhenderson]

+1 to llvm-libtool-darwin. I think avoiding a name clash with GNU libtool in the future is highly desirable. If the two tools are unrelated, then using two different directories makes a lot of sense to me.

The pre-merge bot claims your help test is failing. You'll need to fix that.

In D82923#2126187, @smeenai wrote:

Looking at libtool's options further, the only concern I have is with:

-      Treat all remaining arguments as names of files (or archives) and not as options.

In other words, if you include a single - by itself on your command line, all remaining arguments are parsed as input names. cl::opt supports similar functionality, but using -- instead of -. I don't know if the TableGen parsing can support that either though, so I don't know if it would be possible for us to keep the exact same command line interface (with respect to this flag) anyway.

Thanks. To be clear, I fully support using the tablegen style if it will make things easier - @sameerarora101, you might want to experiment with this issue @smeenai has highlighted before this gets landed, to see which works better.

Also, if you haven't already, you might want to email that mailing thread/post a new email on llvm-dev highlighting that this review now exists so that any interested parties can be aware and chip in if they want.

I'd recommend starting a doc in the CommandGuide folder, as part of this change, referenced from the main page, and updating it as you go, rather than having to write it all at once. That way users will know how much this tool supports.

llvm/tools/llvm-libtool-darwin/LLVMBuild.txt

1

This line doesn't look quite right. It looks too long and might need fixing. Is it normal to include the path here also? That seems a little weird to me.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

13–24

You probably can get rid of most of these.

27

Again, this is redundant in this change.

29–30

This is not currently used by anything, so you can postpone adding it.

32–40

If you are going to add the input/output file options, you need test cases showing what is/isn't accepted in terms of number of arguments and option names.

35

Are you sure you want this as OneOrMore?

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/help-message.test

5

Yup, it uses -long-option.

llvm/tools/llvm-libtool-darwin/LLVMBuild.txt

1

oh sorry abt that, I forgot to remove --- from the ends in order to align the line. I'll update this.

But yeah, for every other tool I checked (ar,link,lipo,lto,size) they have the path.

20

yup, I tried just now and it passes. Thanks.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

32–40

ok added.

35

So I was thinking of allowing user to change the output file by passing in another -o option at the end (since we pick the last one here). But I just checked and cctool's libtool doesn't allow this. So, in order to keep the behavior same, i'll change this to cl::Required. Thanks

[sameerarora101]

Thanks. To be clear, I fully support using the tablegen style if it will make things easier - @sameerarora101, you might want to experiment with this issue @smeenai has highlighted before this gets landed, to see which works better.

Yup, I talked to @smeenai about it. Since tablegen also wouldn't be able to handle - case, I'll simply use cl::opt for now.

Also, if you haven't already, you might want to email that mailing thread/post a new email on llvm-dev highlighting that this review now exists so that any interested parties can be aware and chip in if they want.

Ok, I'll send an email with a link to this review.

I'd recommend starting a doc in the CommandGuide folder, as part of this change, referenced from the main page, and updating it as you go, rather than having to write it all at once. That way users will know how much this tool supports.

Added now, thanks.

[sameerarora101]

Removing redundant includes and adding basic documentation

[sameerarora101]

Here is how the doc looks:

[jhenderson]

llvm/docs/CommandGuide/index.rst

23 ↗

(On Diff #275133)

Would you mind, whilst you are modifying this list, putting this list in alphabetical order? I think it makes more sense that way.

llvm/docs/CommandGuide/llvm-libtool-darwin.rst

24–26 ↗

(On Diff #275133)

I suspect if you do -help you'll see one or two other options too. You'll probably want to include -help-list here at least, and might need to hide some other options. Could you post (out-of-line) what the full help text is, please?

40 ↗

(On Diff #275133)

It might also be worth adding a "See Also" section for llvm-ar since the two tools are somewhat related.

llvm/test/tools/llvm-libtool-darwin/help-message.test

5

You might want to add a -help version here too.

11

I believe this should include error: as a prefix? Please add it, if so. Same applies in the other test.

llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test

1 ↗

(On Diff #275133)

I'd dedicate this test to specifically input/output file arguments. Perhaps rename it to invalid-input-output-args.test

4 ↗

(On Diff #275133)

Use %t or similar, or possibly /dev/null, here, even if the tool shouldn't actually write anything, to avoid any remote risk of it writing to the wrong place.

22 ↗

(On Diff #275133)

Same as above - use a %t variant or /dev/null.

25 ↗

(On Diff #275133)

You probably want a test that shows that the expected passing cases also work (i.e. exit with code 0 for now). This could probably be a test file called basic.test for now. You can replace or expand the test as you add useful functionality. I'd expect the test to have cases for both one input and multiple inputs.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

23–24

A common thing you may not be aware of is that cl::opt aliases don't appear in the help text by default. I imagine this is not desirable for you, so you probably want to add a cl::NotHidden here (I think that's the right term anyway).

33

I don't think you need to explicitly return EXIT_SUCCESS. That's the default behaviour of main in C++.

[sameerarora101]

llvm/docs/CommandGuide/llvm-libtool-darwin.rst

24–26 ↗

(On Diff #275133)

here is the full help text from cmd line:

OVERVIEW: llvm-libtool-darwin

USAGE: llvm-libtool-darwin [options] <input files>

OPTIONS:

Color Options:

  --color             - Use colors in output (default=autodetect)

General options:

  -o                  - Alias for --output
  --output=<filename> - Specify output filename

Generic Options:

  --help              - Display available options (--help-hidden for more)
  --help-list         - Display list of available options (--help-list-hidden for more)
  --version           - Display the version of this program

I have added description for --help-list and --color now as well

40 ↗

(On Diff #275133)

Makes sense, thanks!

llvm/test/tools/llvm-libtool-darwin/help-message.test

11

No, the output doesn't have error: as a prefix. Here is the output for passing -abcabc:

llvm-libtool-darwin: Unknown command line argument '-abcabc'.  Try: './bin/llvm-libtool-darwin --help'

error: is also not there as a prefix for the other test.

llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test

25 ↗

(On Diff #275133)

Ok, I added basic.test where I simply run

# RUN: yaml2obj %S/Inputs/input1.yaml -o %t-input1.o
# RUN: yaml2obj %S/Inputs/input2.yaml -o %t-input2.o

## Pass single input:
# RUN: llvm-libtool-darwin -o %t.lib %t-input1.o

## Pass multiple inputs:
# RUN: llvm-libtool-darwin -o %t.lib %t-input1.o %t-input2.o

Is this sufficient? Or is there some other way I need to verify that the exit code was indeed 0?

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

23–24

thanks

[sameerarora101]

Updating help information; adding basic.test and reordering index.rst

[jhenderson]

llvm/docs/CommandGuide/llvm-libtool-darwin.rst

24–26 ↗

(On Diff #275133)

Thanks. Some more comments:

1. As this is a Darwin-inspired tool, we should use the standard option naming throughout. If I understand it correctly, this means single dashes rather than double.
2. You probably want to use the documentation for the various common options (help, version, color etc) used in the other tool documentation, for consistency. Take a look at e.g. llvm-objcopy or llvm-dwarfdump. In particular, I wouldn't report the "hidden" versions of the help options (they're hidden for a reason...).
3. Documentation should use full English grammar rules with leading caps and trailing full stops, like comments in the code.

llvm/test/tools/llvm-libtool-darwin/basic.test

1 ↗

(On Diff #275574)

Nit: use '##' for comments.

llvm/test/tools/llvm-libtool-darwin/help-message.test

11

Yuck, okay. Maybe something to look at another time, I guess.

llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test

25 ↗

(On Diff #275133)

That's sufficient, thanks. If a tool returns a non-zero exit code, lit will automatically fail unless not is prepended.

llvm/test/tools/llvm-libtool-darwin/invalid-input-output-args.test

1 ↗

(On Diff #275574)

On further reflection, perhaps it makes sense to combine this and basic.test into a single test. What do you think?

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

23

Similar to my documentation comment, I'm okay with this using single-dash if it's more common on Darwin.

[jhenderson]

llvm/test/tools/llvm-libtool-darwin/invalid-input-output-args.test

1 ↗

(On Diff #275574)

Actually, ignore my previous comment, since basic.test is only short-term.

You'll probably want to add --static to these tests when you add support to that option to avoid any potential confusion.

[sameerarora101]

llvm/docs/CommandGuide/llvm-libtool-darwin.rst

24–26 ↗

(On Diff #275133)

Ok, I have replaced -- with - and took help from llvm-dwarfdump for the common options.

llvm/test/tools/llvm-libtool-darwin/invalid-input-output-args.test

1 ↗

(On Diff #275574)

Ya I agree, basic.test is temporary and I replace it with create-static-lib.test when adding the -static option.

[sameerarora101]

Updating docs

[jhenderson]

LGTM, but please give others a chance to review too.

[smeenai]

LGTM.

llvm/docs/CommandGuide/llvm-libtool-darwin.rst

17 ↗

(On Diff #275715)

Nit: The package name is "cctools", so you should say "cctools'" (trailing apostrophe) instead of "cctool's".

llvm/test/tools/llvm-libtool-darwin/basic.test

1 ↗

(On Diff #275715)

Nit: just spell out "error code" instead of abbreviating it as EC.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

19

As far as I can see, cctools libtool doesn't support the -output spelling, only -o. Is there any reason for us to support it?

[sameerarora101]

llvm/docs/CommandGuide/llvm-libtool-darwin.rst

17 ↗

(On Diff #275715)

good catch, thanks 😊

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

19

Yup, that is true. I was just looking at other llvm tools and they have -output in addition to -o. So I thought of adding both. I can remove -output if you guys prefer that?

[sameerarora101]

@smeenai I have also moved OptionCategory and hide-unrelated-options.test to this diff instead of D83002 that adds the support for -static. Thanks

[sameerarora101]

Add OptionCategory and hide-unrelated-options.test

[smeenai]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

19

I'd prefer to remove it, to mimic cctools libtool's interface as closely as possible.

[jhenderson]

llvm/test/tools/llvm-libtool-darwin/hide-unrelated-options.test

1 ↗

(On Diff #276422)

This is probably fine, but an alternative that might be more consistent is to expand help-message.test to show that the option categories that should be supported are printed (by checking the e.g. "Color options:" text), and that the unrelated options aren't (by similarly checking that the header for them isn't included). There are some examples of this for some tools like llvm-size. You might also want to add a --help-list test case as in llvm-size's help.test.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

19

No issues removing -output from me.

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/hide-unrelated-options.test

1 ↗

(On Diff #276422)

ok, I have modeled help.test under llvm-libtool-darwin in a similar fashion as llvm-size. Thanks! (I have also removed hide-unrelated-options.test now).

[sameerarora101]

Remove --output option (keeping -o) and update help.test

[jhenderson]

llvm/test/tools/llvm-libtool-darwin/help-message.test

6

It's quite possible the llvm-size test doesn't have testing for unrelated options. How is it tested in this version now?

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/help-message.test

6

yup, you are right, llvm-size doesn't test that unrelated options are not present. For my case, the unrelated option --safepoint-ir-verifier-print-only: comes under General Options: when I add the support for -static. I can add --implicit-check-not=General in the second diff D83002 for that?

[jhenderson]

llvm/test/tools/llvm-libtool-darwin/help-message.test

6

I think we've done something similar elsewhere. Take a look around, and try seeing if there's a precedent.

I might be inclined for a more verbose --implicit-check-not for "General Options:" for safety (since "General" might appear in somebdoy's path).

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/help-message.test

6

ok got it, thanks!

llvm/test/CMakeLists.txt

set(LLVM_TEST_DEPENDS
           llvm-exegesis
           llvm-extract
           llvm-gsymutil
           llvm-isel-fuzzer
           llvm-ifs
           llvm-install-name-tool
           llvm-jitlink
           llvm-lib
+          llvm-libtool-darwin
           llvm-link
           llvm-lipo
           llvm-locstats
           llvm-lto2
           llvm-mc
           llvm-mca
           llvm-ml
           llvm-modextract

llvm/test/tools/llvm-libtool-darwin/help-message.test

+## This test checks that the help message is displayed correctly.
+
+# RUN: llvm-libtool-darwin -h | FileCheck --check-prefix=LIBTOOL-USAGE %s --match-full-lines
+# RUN: llvm-libtool-darwin --help | FileCheck --check-prefix=LIBTOOL-USAGE %s --match-full-lines
+# RUN: not llvm-libtool-darwin -abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
+# RUN: not llvm-libtool-darwin --abcabc 2>&1 | FileCheck --check-prefix=UNKNOWN-ARG %s
+
+# LIBTOOL-USAGE: USAGE: llvm-libtool-darwin [options] <input files>
+
+# UNKNOWN-ARG: Unknown command line argument '{{-+}}abcabc'

llvm/tools/llvm-libtool-darwin/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  Object
+  Option
+  Support
+  )
+
+add_llvm_tool(llvm-libtool-darwin
+  llvm-libtool-darwin.cpp
+)

llvm/tools/llvm-libtool-darwin/LLVMBuild.txt

+;===- ./tools/llvm-libtool-darwin/LVMBuild.txt --------------------------*- Conf -*--===;
+;
+; Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+; See https://llvm.org/LICENSE.txt for license information.
+; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+;
+;===------------------------------------------------------------------------===;
+;
+; This is an LLVMBuild description file for the components in this subdirectory.
+;
+; For more information on the LLVMBuild system, please see:
+;
+;   http://llvm.org/docs/LLVMBuild.html
+;
+;===------------------------------------------------------------------------===;
+[component_0]
+type = Tool
+name = llvm-libtool-darwin
+parent = Tools
+required_libraries = Object Option Support

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

+//===-- llvm-libtool-darwin.cpp - a tool for creating libraries --------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// A utility for creating static and dynamic libraries for Darwin.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Object/Archive.h"
+#include "llvm/Object/ArchiveWriter.h"
+#include "llvm/Object/Binary.h"
+#include "llvm/Object/MachO.h"
+#include "llvm/Object/MachOUniversal.h"
+#include "llvm/Object/ObjectFile.h"
+#include "llvm/Option/Arg.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/WithColor.h"
+
+using namespace llvm;
+using namespace llvm::object;
+
+// The name this program was invoked as.
+static StringRef ToolName;
+
+static cl::opt<std::string> OutputFile("output",
+                                       cl::desc("Specify output filename"),
+                                       cl::value_desc("filename"),
+                                       cl::OneOrMore);
+static cl::alias OutputFileShort("o", cl::desc("Alias for -output"),
+                                 cl::aliasopt(OutputFile));
+
+static cl::list<std::string>
+    InputFiles(cl::Positional, cl::desc("<input files>"), cl::OneOrMore);
+
+int main(int Argc, char **Argv) {
+  InitLLVM X(Argc, Argv);
+  ToolName = Argv[0];
+  cl::ParseCommandLineOptions(Argc, Argv, "llvm-libtool\n");
+
+  return EXIT_SUCCESS;
+}