[PATCH] D83002: [llvm-libtool-darwin] Add support for -static option

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 18 inline comments as done.
sameerarora101 added inline comments.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:15
+# CHECK-NAMES-NEXT: input2.o
+# CHECK-NAMES-NOT:  {{.}}
+
----------------
jhenderson wrote:
> Watch out, here and in other cases, this only shows that there is no output AFTER `input2.o`. It's possible that there's input before `input1.o`. Also, you'll not catching name corruptions resulting in a prefix/suffix of the name, since FileCheck only does sub-string matching by default, not full line matching. For example, this would fail if the following was emitted:
> 
> ```
> input-I-really-shouldn't-be-here
> input1.osuffix
> prefixinput2.o
> ```
> 
> You probably want to add `--implicit-check-not={{.}}` to the FileCheck command line, rather than the final `CHECK-NAMES-NOT`.
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`


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:19
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-SYMBOLS
+
----------------
jhenderson wrote:
> It would be best to check the symbol is in the right member. You can do this by using FileCheck's -D option, combined with the `%t_basename` (NB: check the exact name for correctness, but there should be other examples):
> 
> ```
> # RUN: ... FileCheck %s -D FILE1=%t_basename ...
> 
> # CHECK-SYMBOLS: _symbol1 in [[FILE1]]
> ```
> 
> and so on. This defines a string variable that matches the specified input string, and can be used using the `[[VAR]]` syntax as shown. Take a look at the FileCheck documentation for details.
> 
> Also, you should check the `Archive map` string to ensure there's no symbol before the first.
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?


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:7
+
+# MISSING-OPERATION: Library Type:  option: must be specified at least once!
+
----------------
jhenderson wrote:
> Does the double space match the actual error message?
Yes, the actual error msg also has the double space:
```
Library Type:  option: must be specified at least once!
```


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:9-18
+## Input file not found:
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t.missing 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NO-FILE -DFILE=%t.missing
+
+# NO-FILE: '[[FILE]]': {{[nN]}}o such file or directory
+
+## Input file is not an object file:
----------------
jhenderson wrote:
> These two checks plus the ELF one below probably belong in the invalid input/output arguments test.
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:`


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:20
+
+# NOT-OBJECT: The file was not recognized as a valid object file
+
----------------
jhenderson wrote:
> Does this message use `error:` as a prefix?
yup, I added `error:` in the error message too now, thanks!


================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:34-38
+static cl::opt<Operation> LibraryOperation(
+    cl::desc("Library Type: "),
+    cl::values(
+        clEnumValN(Static, "static",
+                   "Produce a statically linked library from the input files")),
----------------
jhenderson wrote:
> I'm not really familiar with the `Operation` type. What does it look like in the help text?
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83002/new/

https://reviews.llvm.org/D83002