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

Thu Jul 2 01:34:05 PDT 2020

jhenderson added a comment.

Out of time. Note to self: finish review of the rest of the source code and tests.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:3-4
+
+# RUN: yaml2obj %S/Inputs/helloMachO.yaml -o %t-hello.o
+# RUN: yaml2obj %S/Inputs/mainMachO.yaml -o %t-main.o
+
----------------
I assume the idea is that these two YAML files will be used by many different tests in the future? If so, I'd rename them simply to input1.yaml and input2.yaml to emphasise their generality (in particular, the `main` bit suggests it is significant, which I suspect is not true). I'd probably also rename their symbols to `symbol1` and `symbol2` for the same reason.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:10
+## Check that binaries are present:
+# RUN: llvm-ar tv %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-NAMES --implicit-check-not=.o
----------------
`t` is probably sufficient rather than `tv` since you don't check the other fields.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:14
+# CHECK-NAMES: hello.o
+# CHECK-NAMES: main.o
+
----------------
Use -NEXT suffixes where applicable.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:18
+# RUN: llvm-nm %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-SYMBOLS --implicit-check-not=T
+
----------------
`--implicit-check-not=T` sounds flaky to me, as file paths might start appearing in the output. It also won't indicate that there are no other symbols, merely no other global text symbols. What is the actual output here? Also, are you interested in the archive's symbol table or the symbol table of the members, because I believe this might be printing the latter. There's a `--print-armap` option to do the former.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:25
+# RUN: llvm-objdump --macho --archive-headers %t.lib | \
+# RUN:  FileCheck %s --check-prefix=FORMAT --implicit-check-not=.o
+
----------------
Nit: one more space of indentation, please.

Again, I think the --implicit-check-not sounds dangerous to me. You might be better off with something like `FORMAT-EMPTY:` or `FORMAT-NOT: {{.}}` after the last check line.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:27
+
+# FORMAT: __.SYMDEF
+# FORMAT-NEXT: hello.o
----------------
Add some extra spacing to make this line up:

```
# FORMAT:      __.SYMDEF
# FORMAT-NEXT: hello.o
# FORMAT-NEXT: main.o

```

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:32-36
+# RUN: llvm-libtool-darwin -static -o %t.lib %t-main.o %t-hello.o
+# RUN: llvm-ar tv %t.lib | \
+# RUN:   FileCheck %s --check-prefix=OVERWRITE-NAMES --implicit-check-not=.o
+# RUN: llvm-nm %t.lib | \
+# RUN:   FileCheck %s --check-prefix=OVERWRITE-SYMBOLS --implicit-check-not=T
----------------
Similar comments apply here to the lines above.

Also, I feel like showing that the ordering has been reversed is a little subtle. Assuming the tool completely replaces the original archive, I'd add just a single object to the new one, and show that the output only has that one object in it.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:45-48
+# RUN: llvm-ar tv %t.lib | \
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-NAMES --implicit-check-not=.o
+# RUN: llvm-nm %t.lib | \
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-SYMBOLS --implicit-check-not=T
----------------
Same comments as above, re. --implicit-check-not, symbol table etc.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/invalid-arguments.test:1
+## This test checks that an error is thrown in case of invalid argument(s).
+
----------------
I won't review this now, as I think some of it belongs in the first review, plus I'm running low on time for upstream reviewing today!

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:47
+        clEnumValN(Static, "static",
+                   "Produce a statically linked library from the input files"),
+        clEnumValN(Dynamic, "dynamic",
----------------
Can I confirm that the terminology actually is "statically linked library" in Darwin land? I'd typically use simply "static archive" from my ELF background.

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:49
+        clEnumValN(Dynamic, "dynamic",
+                   "Produce a shared dynamic library from the input files")),
+    cl::Required);
----------------
Is "shared dynamic library" unnecessarily verbose, or is that the common terminology? I'd expect either "shared library" or "dynamic library" from my background, but am not familiar with Darwin development.

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:52-54
+static cl::opt<bool>
+    Deterministic("D", cl::desc("Use zeroes for timestamps and UIDs/GIDs"),
+                  cl::init(false));
----------------
Can this option be delayed to a later change? I'd tentatively do it by default in this version.

FWIW, I think LLVM tools typically set deterministic to `true` by default, since it makes testing easier.

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:56-70
+LLVM_ATTRIBUTE_NORETURN static void reportError(Twine Message) {
+  WithColor::error(errs(), ToolName) << Message << "\n";
+  errs().flush();
+  exit(EXIT_FAILURE);
+}
+
+LLVM_ATTRIBUTE_NORETURN static void reportError(StringRef File, Error E) {
----------------
sameerarora101 wrote:
> > 5 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. 
> 
> These error functions themselves are using basic error handling functions available in the Support library (e.g `logAllUnhandledErrors`). If I use `createStringError` available in the support library, I too would have to pass the error object as an argument to `logAllUnhandledErrors`. Wouldn't I be doing something similar to this only then (I copied these error functions from llvm-lipo) ? sorry if I missed something. Thanks
> 
I was referring to the `WithColor::defaultErrorHandler` function, which does the actual printing of the error message and handles the `Error`. There's also ``createFileError` which takes an `Error` and adds a provided file name. You will still need to convert the string to an error using `createStringError`, but could then pass that directly to `defaultErrorHandler`.

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:77
+
+  // throw error if not a valid object file
+  if (Error E = ObjOrErr.takeError())
----------------
Nit: incorrect comment format. (Use capital letter/full stop).

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:78-79
+  // throw error if not a valid object file
+  if (Error E = ObjOrErr.takeError())
+    reportError(Member.MemberName, std::move(E));
+
----------------
More idiomatic and simpler is:

```
if (!ObjOrErr)
  reportError(Member.MemberName, ObjOrErr.takeError());
```

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:81
+
+  // throw error if not in darwin format
+  if (!isa<object::MachOObjectFile>(**ObjOrErr))
----------------
Ditto.

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:125-126
+    break;
+  case Dynamic:
+    reportError("dynamic option is currently unsupported");
+  }
----------------
Rather than add this as an unsupported option, I'd just only permit the `static` mode for now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83002