[PATCH] D113130: [llvm-libtool-darwin] Throw an error if object file names are repeated

Thu Nov 4 01:34:22 PDT 2021

jhenderson added a comment.

> This change does not allow static libraries to be created if any of the object files in that library would have the same file name.

This should be "This change prevents creation of static libraries if any ...". The phrase "does not allow" suggests that the patch does something, but does not make it possible for the static libraries to be created in this situation (implying that a future change would make it possible).

> Note that this apple's cctools libtool allows such static libraries to be created and only throws a warning about the name collision.

I'm not an Apple/Mach-O developer, and don't really know much, if anything, about the platform or the file format, so take my thoughts with a pinch of salt. However, I wanted to question this:

1. For traditional Unix static libraries, the filenames are largely irrelevant - you can have multiple library members without any issue and it'll just work at link time. This is because such library members are picked based on their symbol names, rather than anything to do with their filenames.
2. If the existing cctools libtool reports a warning rather than an error, why not do the same here? It seems like reporting an error might cause problems where existing users are creating such a library for good reason. NB: if there is no possible good reason, i.e. the library is fundamentally broken, then an error makes sense - see my above point about not being too familiar with the format.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:75
+
+# DUPLICATE-INPUT: error: file '[[FILE]]' was specified multiple times.
+# DUPLICATE-INPUT-DAG: [[INPUTA]]
----------------
We often add extra indentation to make output line up nicely after FileCheck directives of different lengths.

Same in the other tests.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/L-and-l.test:106
+# RUN: not llvm-libtool-darwin -static -o %t.lib -l%basename_t.tmp-input2.o \
+# RUN:  -l%basename_t.tmp-input2.o -L%T 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-INPUT \
----------------
`%T` is generally considered deprecated and may be removed at a future point. This is because the directory is not unique to the test, so there could be mysterious runtime failures due to test ordering issues.

To workaround this, cd into a test-time created directory, with a name derived from `%t`, and then use paths relative to that directory.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/archive-flattening.test:64

+# RUN: not llvm-libtool-darwin -static -o %t.lib %t-x86_64.bc %t.correct.ar %t-input1.o  2>&1| \
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-INPUT -DFILEA=%basename_t.tmp-input1.o \
----------------

================
Comment at: llvm/test/tools/llvm-libtool-darwin/archive-flattening.test:68
+
+# DUPLICATE-INPUT: file '[[FILEA]]' was specified multiple times.
+# DUPLICATE-INPUT-DAG: [[PATHA]]
----------------
I'd recommend including the "error:" prefix here too.

================
Comment at: llvm/test/tools/llvm-libtool-darwin/create-static-lib.test:52

-## Duplicate a binary:
-## cctools' libtool raises a warning in this case.
-## The warning is not yet implemented for llvm-libtool-darwin.
-# 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={{.}}
-# RUN: llvm-ar t %t.lib | \
-# RUN:   FileCheck %s --check-prefix=DUPLICATE-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
-# RUN: llvm-nm --print-armap %t.lib | \
-# RUN:   FileCheck %s --check-prefix=DUPLICATE-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+## Duplicate a binary: This should raise an error.
+# RUN: not llvm-libtool-darwin -static -o %t.lib %t-input1.o %t-input2.o %t-input1.o 2>&1 | \
----------------

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:112
+  // Add a NewArchiveMember and the file it came from to the list.
+  void pushBack(NewArchiveMember &&Member, StringRef File) {
+    Members.push_back(std::move(Member));
----------------
Up to you, but I think for STL-like containers, it's acceptable to follow STL-naming conventions with their methods (i.e. `push_back` here). Others may disagree though :)

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:488-490
+  for (const auto &M : MembersPerArch) {
+    const auto &Members = M.second.getMembers();
+    const auto &Files = M.second.getFiles();
----------------
I'd prefer not to use `auto` here. In fact, for `Members` and `Files`, I believe you could (should?) be using `ArrayRef<>`

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:491
+    const auto &Files = M.second.getFiles();
+    std::map<StringRef, std::vector<StringRef>> MembersToFiles;
+    for (auto iterators = std::make_pair(Members.cbegin(), Files.cbegin());
----------------
Is there a reason this is a) ordered and b) using a `std::map` rather than LLVM's `StringMap`?

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:492
+    std::map<StringRef, std::vector<StringRef>> MembersToFiles;
+    for (auto iterators = std::make_pair(Members.cbegin(), Files.cbegin());
+         iterators.first != Members.cend();
----------------
`Iterators` or simply `Its` (always use UpperCamelCase for variables).

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:494
+         iterators.first != Members.cend();
+         // which should imply iterators.second != Files.cend()
+         ++iterators.first, ++iterators.second) {
----------------
LLVM comments are always full sentences ("This should imply iterators.second != Files.cend()."). I'd suggest using C-style block comments too, so that you aren't forced to put this on a new line. Finally, I'd drop the "should" (i.e. "This implies...".

You might want to add an assertion to sanity check that `iterators.second != Files.cend()`

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:495
+         // which should imply iterators.second != Files.cend()
+         ++iterators.first, ++iterators.second) {
+      MembersToFiles[iterators.first->MemberName].push_back(*iterators.second);
----------------
Throughout this function: LLVM style says no braces for single line ifs and loops.

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:501
+    raw_string_ostream ErrorStream(ErrorData);
+    for (const std::pair<StringRef, std::vector<StringRef>> &pair :
+         MembersToFiles) {
----------------
Probably rename this to `MemberToFiles`, rather than the rather meaningless "pair".

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:505
+        ErrorStream << "file '" << pair.first.str()
+                    << "' was specified multiple times.\n";
+
----------------
LLVM coding standards say that error messages shouldn't end in a `.`

================
Comment at: llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp:540-542
+  if (Error E = checkForDuplicates(NewMembers)) {
+    return E;
+  }
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113130