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

[Sameer Arora via Phabricator via llvm-commits]

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


================
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
+
----------------
jhenderson wrote:
> `--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.
you are right, `--print-armap` is what I wanted. It prints:
```
Archive map
_symbol1 in create-static-lib.test.tmp-input1.o
_symbol2 in create-static-lib.test.tmp-input2.o


/data/users/<complete-path>/create-static-lib.test.tmp.lib(create-static-lib.test.tmp-input1.o):
0000000000000000 T _symbol1

/data/users/<complete-path>/create-static-lib.test.tmp.lib(create-static-lib.test.tmp-input2.o):
0000000000000000 T _symbol2
```

I have now replaced the check with : 
```
# CHECK-SYMBOLS:      _symbol1 in
# CHECK-SYMBOLS:      _symbol2 in
# CHECK-SYMBOLS-EMPTY:
```
Would this be fine?
Thanks


================
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
----------------
jhenderson wrote:
> 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.
alrighty, replaced the test by adding a single object to it.


================
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",
----------------
jhenderson wrote:
> 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.
Yup, I took this from the man pages (https://www.unix.com/man-page/osx/1/libtool/):

> Libtool can create either dynamically linked shared libraries, with -dynamic, or statically linked (archive) libraries, with -static.


================
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);
----------------
jhenderson wrote:
> 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.
For the dynamic option the man page says:

> -dynamic
> Produce a dynamically linked shared library from the input files.

But I can update our description to something else too ?

(PS: I have removed the dynamic option for this version)


================
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));
----------------
jhenderson wrote:
> 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.
ok, I'll set it to true for now as well (and pass it as a default instead of creating an option).


================
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));
+
----------------
jhenderson wrote:
> More idiomatic and simpler is:
> 
> ```
> if (!ObjOrErr)
>   reportError(Member.MemberName, ObjOrErr.takeError());
> ```
thanks.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83002