Page MenuHomePhabricator

[llvm-libtool-darwin] Add support for -static option
ClosedPublic

Authored by sameerarora101 on Jul 1 2020, 3:09 PM.

Details

Summary

Add support for creating static libraries when the input
includes only binaries (and not libraries/archives themselves)
Depends on D82923

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sameerarora101 marked 18 inline comments as done.Jul 6 2020, 11:05 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
16

Thanks, I added --implicit-check-not. Furthermore, I also added -DPREFIX=create-static-lib.test.tmp as the file names are represented by [[PREFIX]]-input1.o and [[PREFIX]]-input2.o

20

Yup, I now check that the symbol is in the right member using the following:

## Check that symbols are present:
# RUN: llvm-nm --print-armap %t.lib | \
# RUN:   FileCheck %s --check-prefix=CHECK-SYMBOLS -DPREFIX=create-static-lib.test.tmp

# CHECK-SYMBOLS:      Archive map
# CHECK-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
# CHECK-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
# CHECK-SYMBOLS-EMPTY:

Would this be ok?

llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test
7 ↗(On Diff #275578)

Yes, the actual error msg also has the double space:

Library Type:  option: must be specified at least once!
9–18 ↗(On Diff #275578)

Ok, I have placed all 4 tests into invalid-input-output-args.test now. Please lemme know in case we needed a separate test file for the first test above ## Missing -static option:

20 ↗(On Diff #275578)

yup, I added error: in the error message too now, thanks!

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
35–39

this is what help text looks like:

OVERVIEW: llvm-libtool-darwin

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

OPTIONS:

Color Options:

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

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

llvm-libtool-darwin options:

  -o                  - Alias for -output
  --output=<filename> - Specify output filename
  Library Type: 
      --static           - Produce a statically linked library from the input files

I created an enum Operation here so that in future we can add support for dynamic operation easily. I can very well make the -static option a boolean flag as well. What do you think?

sameerarora101 marked 6 inline comments as done.

Updating tests

sameerarora101 marked an inline comment as done.Jul 6 2020, 12:41 PM
sameerarora101 added inline comments.
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
78

@jhenderson Should I replace the type of Member back to auto. clang-tidy raises a warning with StringRef?

jhenderson added inline comments.Jul 7 2020, 12:33 AM
llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
12

It's not widely used, but there is %basename_t which substitutes for just the file name part of %t. This allows the test to not make assumptions about how %t expands, and also keeps the check independent of the test name. I'd recommend trying that.

19

Mostly fine, but use %basename_t here too.

35

Checking -> Check

38

Here and below, use %basename_t too, to avoid baking in file names.

45

Ditto.

51–53

Ditto.

llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test
7 ↗(On Diff #275578)

Okay, bonus marks for fixing it in another patch if you want. Or file a bug against the CommandLine code.

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

Maybe make this more generic, e.g. "Missing library type option:"

I don't really know where to put this test. It might want to be its own test case entirely, e.g. "missing-library-type.test"

44

Use %basename_t here too.

51

Use %basename_t here too.

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

Adding the categories sounds like a different change to me? You might want to include it alongside a test case to show that unrelated options aren't incldued.

35–39

Your current approach seems fine. I was just making sure it didn't do anything too weird.

78

Well typically you don't want to use a const & for StringRef because it is already a light-weight construct (much the same as you wouldn't use const & to a pointer or primitive type). That is probably the cause of the problem here.

98

See above re. option category change.

sameerarora101 marked 19 inline comments as done.Jul 7 2020, 8:18 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
12

thanks, I now have -DPREFIX=%basename_t.tmp as %basename_t substitutes for the last path component of %t but without the .tmp extension

llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test
7 ↗(On Diff #275578)

😂I can fix it in another patch.

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

Isn't it similar to ## Missing output file? (That's why I thought it would go here). But I have placed it in missing-library-type.test for now.

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

yup, I added OptionCategory in order to prevent the general option --safepoint-ir-verifier-print-only from showing up in the help text. Added a test case for it as well. Thanks

78

ok thanks

sameerarora101 marked 5 inline comments as done.

Use %basename_t and add test for unrelated options.

smeenai added inline comments.Jul 7 2020, 6:20 PM
llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
19

Might wanna add --match-full-lines here, to ensure there's no unexpected symbol name prefix or member name suffix.

33

You have an underscore instead of a dash :)

Is the purpose to ensure that there's no other members? I assume a -EMPTY would work for that. We should also check for the "Archive : " header, to ensure there's no members before the table of contents member.

48

Might be worth noting that cctools libtool warns in this case, and we don't implement that warning yet.

llvm/test/tools/llvm-libtool-darwin/hide-unrelated-options.test
1 ↗(On Diff #276081)

This seems unrelated to this diff; perhaps it should be in the previous one?

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

Can you add -static to all of the tests in this file, so that the only invalid part of the command line is the aspect being tested?

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

Yup, this should either go in the previous diff or be its own diff.

Mostly looks good, barring @smeenai's comments.

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

Similar in concept, but different behaviour (it's checking the behaviour of the cl::Required on the Library operation option, which is a different thing to the input and output files).

llvm/test/tools/llvm-libtool-darwin/missing-library-type.test
4–6

I don't think it's necessary to add --strict-whitespace here, as the exact formatting of the message isn't really the tool's responsibility. I'd get rid of the double space, as if the message was correct, and the option, so that you don't have to modify this test when you fix the command line message.

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

You can get rid of the const too if you want. StringRef itself is immutable, so all this does is stop you re-assigning Member.

sameerarora101 marked 11 inline comments as done.Jul 8 2020, 7:59 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
33

Thanks for catching the underscore.

I added a check for "Archive : " header now. However, using FORMAT-EMPTY would just check that the next line (after 2nd member) has nothing on it. What I thought we wanted to check was that there is nothing at all after the second member. For eg,

Archive : ...
... __.SYMDEF
...<prefix>-input1.o
...<prefix>-input2.o

something here

would pass with FORMAT-EMPTY just below the check for second member, right? But we want it to fail.

llvm/test/tools/llvm-libtool-darwin/hide-unrelated-options.test
1 ↗(On Diff #276081)

Yup, thanks. Moved it to the previous diff D82923

sameerarora101 marked 2 inline comments as done.Jul 8 2020, 7:59 AM

Updating tests

smeenai added inline comments.Jul 8 2020, 8:04 PM
llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
33

You're right, that makes sense.

smeenai accepted this revision.Jul 8 2020, 8:06 PM

LGTM, but please wait for @jhenderson too.

This revision is now accepted and ready to land.Jul 8 2020, 8:06 PM
jhenderson accepted this revision.Jul 9 2020, 12:26 AM

LGTM, with one optional suggestion.

llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
52

It might be worth adding a 2&>1 and check the output is empty here, to flag up if a warning starts getting emitted. That way, it points to where to add testing for the warning.

sameerarora101 marked 2 inline comments as done.Jul 9 2020, 8:43 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
52

ok, added

# RUN: llvm-libtool-darwin -static -o %t.lib %t-input1.o %t-input2.o %t-input1.o 2>&1 | \
# RUN:   FileCheck %s --allow-empty --implicit-check-not={{.}}

I hope --alow-empty is the right way to go about empty input files (found it here http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20140804/229905.html). The flag is not there in the documentation

sameerarora101 marked an inline comment as done.

Updating test files

sameerarora101 marked an inline comment as done.Jul 9 2020, 8:56 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/help-message.test
10

added --implicit-check-not="--safepoint-ir-verifier-print-only as we know the headers won't be present because of LIST-NOT checks below.

alexshap added inline comments.Jul 9 2020, 6:21 PM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
34

enum class
+ i would add a blank line between the lines 34 and 35

sameerarora101 marked an inline comment as done.Jul 9 2020, 8:15 PM

enum -> enum class

As per @alexshap's recommendation, verifyDarwinObject -> verifyMachOObject

jhenderson added inline comments.Jul 13 2020, 12:48 AM
llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
26

If I'm not mistaken, the canonical name is "Mach-O" (we use MachO for variable and tool names etc, but the file format is technically Mach-O, so we should use that in comments etc).

52

Sounds like a documentation bug. Feel free to update the doc if you get a minute. By default, FileCheck emits an error if the input is empty, but this can be overridden with the use of --allow-empty. That in turn has got me wondering whether --allow-empty should be replaced by a new option that also implies --implicit-check-not. I've raised that on llvm-dev.

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

Sounds reasonable, but I'd get rid of the -- bit of the implicit-check-not pattern, so that it doesn't matter whether options are printed in the help with one or two dashes.

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

Mach-O (same as mentioned elsewhere).

73

Ditto.

sameerarora101 marked 5 inline comments as done.

MachO -> Mach-O in comments and update help-message.test

sameerarora101 marked an inline comment as done.Jul 13 2020, 7:13 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
52

ok, I'll create a diff for the doc, thanks!

sameerarora101 marked an inline comment as done.Jul 13 2020, 7:50 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/create-static-lib.test
52

added it here : D83682

jhenderson accepted this revision.Jul 14 2020, 12:36 AM

LGTM again.

Mach-O -> Darwin for archive format in comments

This revision was automatically updated to reflect the committed changes.