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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:15
+# CHECK-NAMES-NEXT: input2.o
+# CHECK-NAMES-NOT:  {{.}}
+
----------------
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`.


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


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:22
+# CHECK-SYMBOLS:      _symbol1 in
+# CHECK-SYMBOLS:      _symbol2 in
+# CHECK-SYMBOLS-EMPTY:
----------------
Use `CHECK-SYMBOLS-NEXT` here to show there's no symbol in between the two symbols.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:41-42
+
+# OVERWRITE-NAMES:        input2.o
+# OVERWRITE-NAMES-NOT:    {{.}}
+
----------------
This has the same issue as the earlier comment re. `CHECK-NOT`/`--implicit-check-not`

In fact, if you used OVERWRITE-NAMES and OVERWRITE-SYMBOLS instead of CHECK-NAMES/CHECK-SYMBOLS above, the test will still pass.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:44-45
+
+# OVERWRITE-SYMBOLS:      _symbol2 in
+# OVERWRITE-SYMBOLS-EMPTY:
+
----------------
Same as above.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:54-62
+# DUPLICATE-NAMES:     input1.o
+# DUPLICATE-NAMES:     input2.o
+# DUPLICATE-NAMES:     input1.o
+# DUPLICATE-NAMES-NOT: {{.}}
+
+# DUPLICATE-SYMBOLS:      _symbol1 in
+# DUPLICATE-SYMBOLS:      _symbol2 in
----------------
Same comments here as above.


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


================
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:
----------------
These two checks plus the ELF one below probably belong in the invalid input/output arguments test.


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:20
+
+# NOT-OBJECT: The file was not recognized as a valid object file
+
----------------
Does this message use `error:` as a prefix?


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:24
+# RUN: yaml2obj %s -o %t
+# RUN: not llvm-libtool-darwin -static -o $t.lib %t 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOT-MACHO
----------------
Did you mean to use `$t.lib`? (probably not)


================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:29-34
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
----------------
Only these lines are required to emit a minimal ELF object. Delete everything else. Also, we prefer to use minimal amounts of padding:

```
--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_X86_64
```


================
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")),
----------------
I'm not really familiar with the `Operation` type. What does it look like in the help text?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83002