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

[Leonard Grey via Phabricator via llvm-commits]

lgrey added inline comments.


================
Comment at: lld/MachO/InputFiles.cpp:1267
+  // symbols later in the link stage). So we append file offset to make
+  // filename unique.
+  MemoryBufferRef mbref(
----------------
thakis wrote:
> 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?
It's both (when I ran into this with Chrome, it was two different archives). Rejiggered the comment a little (part about collision is no longer true since it will just error inside LTO); does it make more sense now?


================
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:]]
----------------
thakis wrote:
> Do you even need this? If the link succeeded, it means we pulled in both .o files
Agree, but this applies to all the FileCheck stuff in here. Do you think it's worth keeping as documentation, or should I just run the links, add a comment re: how they would fail, and call it a day?


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

https://reviews.llvm.org/D106179