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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: lld/test/MachO/Inputs/archive4.s:1
+.global _lonely
+_alone:
----------------
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 :)


================
Comment at: lld/test/MachO/archive.s:15
+# CHECK-NEXT: T _boo
+# CHECK-NEXT: T _main
+
----------------
Ktwu wrote:
> smeenai wrote:
> > We'll wanna add a `CHECK-NOT` for `_lonely`/`_alone` (depending on what you decide to call that symbol), to make sure that archive member wasn't pulled in.
> > 
> > Unfortunately, that depends on the order symbols are printed out by nm, so it's not completely reliable. As in, if we did have `_alone` in the symbol table (because we incorrectly pulled in that archive member), and nm printed it out before `_bar`, having a `CHECK-NOT: T _alone` after this line wouldn't do us any good. Having a second `CHECK-NOT` before the `CHECK` for `_bar` should be enough to make this work, I think.
> I can also rerun `llvm-nm %t.out` explicitly to make sure that the symbols don't appear anywhere, right? That way I wouldn't have to worry about ordering.
Yeah, that's better!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78342