[PATCH] D106179: [lld-macho] Disambiguate bitcode files with the same name by archive name/offset in archive

[Nico Weber via Phabricator via llvm-commits]

thakis accepted this revision.
thakis added a comment.

Generally lg, but I'm confused by the comment (I think I unconfused myself though), and some nits about the test. Thanks!



================
Comment at: lld/MachO/InputFiles.cpp:1267
+  // symbols later in the link stage). So we append file offset to make
+  // filename unique.
+  MemoryBufferRef mbref(
----------------
The code adds the archiveName now, which should disambiguate two archives having the same name. Why do we need the offset too? 

Looks like the offset used to be added only if archiveName is empty and D75422 changed that.

But when can the archive name be empty?

...aha, D60549 that added this to the COFF port has two obj files with the same name in a single archive. So for that including the offset makes sense. Oh, you have that test too :) So it's just the comment that's confusing. It should say "If one archive defines two members with the same name", right?


================
Comment at: lld/test/MachO/lto-archivecollision.ll:3
+; RUN: mkdir %t/a %t/b
+; RUN: opt -thinlto-bc -o %t/main.obj %t/main.ll
+; RUN: opt -thinlto-bc -o %t/a/bar.obj %t/foo.ll
----------------
.obj is a windows name. use .o.


================
Comment at: lld/test/MachO/lto-archivecollision.ll:7
+; RUN: llvm-ar crs %t/libbar.a %t/a/bar.obj %t/b/bar.obj
+; RUN: %lld  -save-temps %t/main.obj %t/libbar.a -o %t/test
+; RUN: FileCheck %s --check-prefix=SAME-ARCHIVE < %t/test.resolution.txt
----------------
nit: one space is enough


================
Comment at: lld/test/MachO/lto-archivecollision.ll:11
+; RUN: opt -thinlto-bc -o %t/a/bar.obj %t/foo.ll
+; RUN: opt -thinlto-bc -o %t/b/bar.obj %t/bar.ll
+; RUN: llvm-ar crs %t/liba.a %t/a/bar.obj
----------------
No need to do this again, `%t/a/bar.o` and `%t/b/bar.o` are still around from the previous test


================
Comment at: lld/test/MachO/lto-archivecollision.ll:19
+; SAME-ARCHIVE-NEXT: -r={{.*}}/libbar.abar.obj[[#OFFSET:]],_foo,p
+;; FIXME: Find a way to assert OTHEROFFSET != OFFSET
+; SAME-ARCHIVE-NEXT: libbar.abar.obj[[#OTHEROFFSET:]]
----------------
Do you even need this? If the link succeeded, it means we pulled in both .o files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106179