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

Wed May 13 14:10:10 PDT 2020

smeenai added a comment.

Looks good with all the comments addressed! Making the test not run a binary is the only real blocker at this point, I think.

================
Comment at: lld/MachO/InputFiles.cpp:287
+  auto file = make<ObjFile>(mb);
+  sections.insert(sections.end(), file->sections.begin(), file->sections.end());
+  return file;
----------------
This is much nicer!

There's still some copying going on here, but given that it's just a bunch of pointers, the copying should be pretty fast. I think this is good enough for now, and we can think about how to make this better in a follow-up. (My cause for concern is that with subsections via symbols implemented, some object files could end up with a sizeable number of sections, so even copying all the pointers over could have a non-trivial cost.)

================
Comment at: lld/MachO/Symbols.cpp:18

+InputFile *LazySymbol::fetch() { return cast<ArchiveFile>(file)->fetch(sym); }
+
----------------
int3 wrote:
> this method seems currently unused
Not sure which iteration the comment was from, but it's used by SymbolTable::addUndefined above. The return value is unused though ... perhaps we could omit that if it's not gonna be useful in the future?

I'd also prefer a name like `fetchMember` or `fetchArchiveMember`, since I believe "member" is the standard terminology for a file in an archive, but that's not a super strong preference.

================
Comment at: lld/test/MachO/Inputs/archive4.s:1
+.global _lonely
+_alone:
----------------
smeenai wrote:
> Ktwu wrote:
> > smeenai wrote:
> > > Directive doesn't match symbol name below
> > This was on purpose; I thought it'd be useful to have both a declaration and a definition that don't link up to anything.
> > 
> > If that's not actually useful (or if I should be explicit), I'll change it.
> Got it. I didn't realize that `.global` by itself would add a reference to that symbol in the symbol table. This looks good!
> 
> To elaborate further on what I meant, I thought that a `.global` would just be ignored if it named a symbol you didn't define in the file. Instead, it looks like it adds an undefined symbol with that name to the symbol table, so essentially serving as a declaration (as you put it). Today I learned :)
Sorry, might not have been clear enough in my above comment. I meant that your original (where the directive and the label had different names) was fine as-is. This works too, of course.

================
Comment at: lld/test/MachO/archive.s:9
+# RUN: llvm-ar rcs %t.a %t2.o %t3.o %t4.o
+# RUN: lld -flavor darwinnew %t.o %t.a -o %t.out
+
----------------
Can you also add a test for when the archive appears on the linker command line before the object file? The results should be identical.

================
Comment at: lld/test/MachO/archive.s:18
+# RUN: llvm-nm %t.out | FileCheck %s --check-prefix VISIBLE
+# VISIBLE-NOT: T _alone
+# VISIBLE-NOT: T _lonely
----------------
`_alone` doesn't exist in the current incarnation of `archive4.s`.

================
Comment at: lld/test/MachO/archive.s:3
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %S/Inputs/archive.s -o %t.main
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %S/Inputs/archive2.s -o %t2
----------------
MaskRay wrote:
> Ktwu wrote:
> > int3 wrote:
> > > Nits: I don't think having a separate `Inputs/archive.s` is necessary; could just include its contents below and reference it  via `%s`.
> > > 
> > > Also, I was looking at some of the lld-ELF tests (e.g. `archive-fetch.s`), and it seems that we can create object files / archives with symbols but no corresponding code. So we could define those files inline too via `echo '.globl _boo | llvm-mc ...`
> > I'll definitely get rid of archive.s (this was back when I didn't know how Filecheck worked).
> > 
> > I like having explicit test files (although knowing about passing stuff straight to llvm-mc is a neat trick), so I'd prefer to keep the archive#.s if that's OK.
> For a definition, just write:
> 
> `echo '.globl _bar; _bar:' | llvm-mc -filetype=obj -triple=x86_64-apple-darwin - -o %t2.o`
> 
> For a reference:
> 
> `echo '.globl _bar' | llvm-mc -filetype=obj -triple=x86_64-apple-darwin - -o %t3.o`
> 
> Use applicable file extensions.
I don't have a super strong preference about separate test files vs. inlining, and there's definitely a point at which inlining becomes unreadable, but for tests where we just need to either define or reference a symbol, the inlining is nice in that you don't have to open up another file to understand what the test is trying to do.

Note that for the input files used by this test, the only thing that should matter is defining a symbol; the contents of the symbol shouldn't matter.

================
Comment at: lld/test/MachO/symbol-order.s:16-17
+
+# RUN: lld -flavor darwinnew %tlibf1.dylib %tlibf2_g.a %t.o -lSystem -o %t.out
+# RUN: ./%t.out; echo $? | FileCheck %s --check-prefix DYLIB-FIRST
+# DYLIB-FIRST: 1
----------------
int3 wrote:
> lld's tests don't typically involve executing the linked binaries (plus it wouldn't work anyway on non-OSX CI machines). `-lSystem` isn't portable either.
> 
> Instead of using constant literals to distinguish where a symbol came from, I'd suggest using custom section names instead. That is, you can have each object file in the archive have the same symbol name under a different section, then see which section it appears under in the final output. `llvm-objdump --syms --macho --lazy-bind` should cover that, plus show if a symbol is coming from the dylib instead.
Yeah, running a binary in a test is a no-go ... we just have to examine the output statically. I like the suggestion for having different section names.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78342