[PATCH] D124895: [Object] Fix updating darwin archives

Thu May 5 08:49:44 PDT 2022

jyknight added a comment.

In D124895#3492632 <https://reviews.llvm.org/D124895#3492632>, @keith wrote:

> In D124895#3491724 <https://reviews.llvm.org/D124895#3491724>, @jyknight wrote:
>
>> Also, while your goal of improving the heuristics seems reasonable, perhaps we ought to have a --archive-format option for objcopy, so that users can be explicit if they desire. (We already have --format for ar, and for the cross-compiling case it's basically required -- there's simply no other way to choose the correct behavior for archives with no files, like `llvm-ar rcs x.a`).
>
> From a UX perspective I hope we can come to a reasonable patch here instead so folks don't have to know about these details in this case. In the case of no files I don't think the decision really matters for this use case at least, although I get that issue in general.

K_DARWIN definitely matters for an empty archive -- ld64 obnoxiously refuses to accept an empty archive unless it has a symbol table. Try the test case: `llvm-ar cr --format=darwin x.a; llvm-objcopy x.a y.a`.

Of course, if ld64 is ever removed in favor of lld, we could stop worrying about most of these issues, and just write normal BSD (or even GNU/GNU64!) archives...except for one bit. The workaround which would need to remain is the duplicate-filename+timestamp avoidance. That's necessitated by the format used for "N_OSO" entries in a linked binary, which is needed so that lldb and dsymutil can lookup archive members to get debug info (as explained in a long comment in ArchiveWriter.cpp.)

> From this comment I also realized I could probably apply the "is first object macho" heuristic only to the decision about what format is needed for 64 bit archives, and therefore I could potentially take the path of removing K_DARWIN if we felt like applying the other darwin specific workarounds to standard BSD archives was also acceptable, do you have any thoughts there? I believe there wouldn't be any functional downsides, as it worked this way in the past before as well, but it adds some unnecessary behavior for non-macho K_BSD archives.

I'd rather not.

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:633-634

+  if (Kind == object::Archive::K_BSD && !NewMembers.empty() &&
+      NewMembers[0].isMacho())
+    Kind = object::Archive::K_DARWIN;
----------------
I don't think this function should be doing this -- I'd like for the two callers in llvm/lib/ObjCopy/Archive.cpp (where it does `deepWriteArchive(..., Ar.kind(), ...)`) and llvm/tools/llvm-ar/llvm-ar.cpp (where it does `Kind = OldArchive->kind();`) to pass a different Kind (computed using some common function placed somewhere appropriate).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124895