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

[Keith Smiley via Phabricator via llvm-commits]

keith added inline comments.


================
Comment at: llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp:487-490
+      auto Kind = (*ArOrErr)->kind();
+      if (Kind == object::Archive::K_BSD && !NewArchiveMembersOrErr->empty() &&
+          isMacho(*NewArchiveMembersOrErr->front().Buf))
+        Kind = object::Archive::K_DARWIN;
----------------
jyknight wrote:
> This ought to specify K_DARWIN unconditionally. An archive inside a Mach-O universal binary wrapper has no reason to be in other formats.
Yea I actually had that originally, but it broke some tests where it manually specified `--flavor=gnu`, I looked at the history of that test and that wasn't the intent of the change in the test, but I figured I'd preserve the behavior. The only case I was potentially worried about with that is if folks were intentionally using thin archives (it would crash in that case at least, instead of doing the entirely wrong thing). I've simplified the logic here to not check the members to preserve the thin case, but I'm happy to just change it to always pass darwin if you think that would still be preferred.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:973-980
+  if (Kind == object::Archive::K_BSD) {
+    if (NewMembersP && !NewMembersP->empty() &&
+        llvm::object::isMacho(*NewMembersP->front().Buf))
+      Kind = object::Archive::K_DARWIN;
+    else if (!NewMembers.empty() &&
+             llvm::object::isMacho(*NewMembers.front().Buf))
+      Kind = object::Archive::K_DARWIN;
----------------
jyknight wrote:
> The special-casing only needs to apply in the "else if (OldArchive)" under "case Default" above.
> 
> 1. If "--format=bsd" was explicitly specified on the command-line, we shouldn't be overruling with a detected format.
> 2. In the other cases in the "if" chain within that, we've computed Kind from getKindFromMember, which already has logic that does something very similar (possibly even the same?
> 
> Actually, not quite sure, but maybe getKindFromMember's detection logic could be unified with the detection logic for isMachO?
Ah yes good point, moved this up there.

I originally went down the path of extracting getKindFromMember for this so it could be used in objcopy as well, but I wasn't sure if we'd want to override the format in the other cases we hit here. This diff has changed a lot though so I pushed that approach here by moving this function to ArchiveMember, let me know what you think!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124895