This is an archive of the discontinued LLVM Phabricator instance.

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

sameerarora101 created this revision.Jun 30 2020, 4:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2020, 4:29 PM

Resolving clang-tidy and test failure on windows

  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.
  1. Is this significantly different to llvm-ar and its aliases? If so, please outline the differences.
  1. 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.
  1. 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.
  1. 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.
  1. 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 added a comment.EditedJul 1 2020, 8:27 AM
  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

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

  1. 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.
  2. 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.
  3. 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

  1. 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!

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

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

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

  1. 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.)

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.

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.

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.

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.

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