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.

[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

6

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

21

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

2

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

14–25

You probably can get rid of most of these.

28

Again, this is redundant in this change.

30–31

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

33–41

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.

36

Are you sure you want this as OneOrMore?

[sameerarora101]

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

6

Yup, it uses -long-option.

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

2

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.

21

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

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

33–41

ok added.

36

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

19–20

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

25–27

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?

41

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

25–27

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

41

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

25–27

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

2

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

2

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

2

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

25–27

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

2

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

18

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

2

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

18

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!