Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

introducing llvm-libtool-darwin
ClosedPublic

Authored by sameerarora101 on Jun 30 2020, 4:29 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sameerarora101 retitled this revision from introducing llvm-libtool to introducing llvm-libtool-darwin.
sameerarora101 edited the summary of this revision. (Show Details)

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 added inline comments.Jul 1 2020, 4:22 PM
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.

+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.

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 marked 13 inline comments as done.Jul 2 2020, 8:05 AM
sameerarora101 added inline comments.
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 marked 5 inline comments as done.Jul 2 2020, 8:09 AM

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.

Removing redundant includes and adding basic documentation

Here is how the doc looks:

jhenderson added inline comments.Jul 3 2020, 1:50 AM
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 marked 16 inline comments as done.Jul 5 2020, 2:05 PM
sameerarora101 added inline comments.
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 updated this revision to Diff 275574.EditedJul 5 2020, 2:07 PM
sameerarora101 marked 5 inline comments as done.

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

jhenderson added inline comments.Jul 6 2020, 1:19 AM
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 added inline comments.Jul 6 2020, 1:23 AM
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 marked 7 inline comments as done.Jul 6 2020, 7:49 AM
sameerarora101 added inline comments.
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 marked an inline comment as done.

Updating docs

jhenderson accepted this revision.Jul 7 2020, 1:31 AM

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

This revision is now accepted and ready to land.Jul 7 2020, 1:31 AM
smeenai accepted this revision.Jul 7 2020, 5:50 PM

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?

MaskRay accepted this revision.Jul 7 2020, 7:09 PM
sameerarora101 marked 5 inline comments as done.Jul 8 2020, 7:07 AM
sameerarora101 added inline comments.
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 marked 2 inline comments as done.EditedJul 8 2020, 7:08 AM

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

Add OptionCategory and hide-unrelated-options.test

smeenai added inline comments.Jul 8 2020, 8:09 PM
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 added inline comments.Jul 9 2020, 12:34 AM
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 marked 4 inline comments as done.Jul 9 2020, 7:32 AM
sameerarora101 added inline comments.
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 marked an inline comment as done.Jul 9 2020, 7:32 AM

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

jhenderson added inline comments.Jul 9 2020, 7:42 AM
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 marked an inline comment as done.Jul 9 2020, 7:52 AM
sameerarora101 added inline comments.
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 added inline comments.Jul 9 2020, 7:56 AM
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 marked 2 inline comments as done.Jul 9 2020, 7:57 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/help-message.test
6

ok got it, thanks!

This revision was automatically updated to reflect the committed changes.
sameerarora101 marked an inline comment as done.