[PATCH] D78342: [lld] Add archive file support to Mach-O backend

[Kellie Medlin via Phabricator via llvm-commits]

Ktwu marked 2 inline comments as done.
Ktwu added inline comments.


================
Comment at: lld/MachO/Driver.cpp:116
+
+    inputFiles.push_back(make<ArchiveFile>(file.release()));
+    break;
----------------
int3 wrote:
> Ktwu wrote:
> > smeenai wrote:
> > > So we're just leaking the memory for the Archive? That's not ideal.
> > > 
> > > LLD ELF has the `unique_ptr<object::Archive>` be a member of `ArchiveFile` and moves the pointer here into the constructor. Can we do the same?
> > I originally had it so that ArchiveFile owned the input archive, but @int3 mentioned passing around unique_ptr is weird, so I made this change and figured leaking was acceptable given the globals in the linker :(
> > 
> > 
> I said passing around unique_ptr is weird if the callee isn't changing ownership... which it wasn't :) (I think the first iteration of the diff was actually allowing the input archive to be freed prematurely... didn't notice that the first time around)
> 
> But yeah, we should follow what LLD-ELF is doing here -- take the Archive by rvalue reference to unique_ptr, and store it in a unique_ptr member in ArchiveFile.
Ahhhh, you're right @int3 (I am sorry for the misattribution, my memory hasn't been keeping up-to-date with this diff's history very well).


================
Comment at: lld/test/MachO/archive.s:18
+
+## Linking with the archive first in the command line shouldn't change anything
+# RUN: lld -flavor darwinnew %t/test.a %t/main.o -o %t/test.out
----------------
smeenai wrote:
> int3 wrote:
> > You could reuse the same CHECK commands instead of having a different prefix with the same checks -- that's what I did in my order file diff. But this is fine too
> Yeah, what I was thinking was that you create two separate output files, one for the archive second case and one for the archive first case, and then run the same FileCheck commands for both (as in with the same check prefix (or lack thereof)). That way it's immediately obvious that both of them have the same output, and the VISIBLE-NOT tests cover both cases as well.
This isn't a blocker, is it? What I've got here is more clear to me than making two output files :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78342