[PATCH] D114841: [lld-macho] Fix duplicate symbols with relocatable objects

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Driver.cpp:260-261
+  // can also lead to duplicate symbol errors.
+  if (InputFile *cachedFile = loadedObjects[path])
+    return cachedFile;
+
----------------
this seems a bit awkward since a) `loadedObjects` now contains both objects and archives and b) we are querying this map even if the file ends up being a dylib. Can we have separate `loadedArchives` and `loadedObjects` maps?


================
Comment at: lld/MachO/Driver.cpp:322-324
   case file_magic::macho_object:
-    newFile = make<ObjFile>(mbref, getModTime(path), "");
+    newFile = loadedObjects[path] = make<ObjFile>(mbref, getModTime(path), "");
     break;
----------------
oontvoo wrote:
> keith wrote:
> > oontvoo wrote:
> > > On a second thought, this is suspicious. Doesn't this mean `%lld foo.o foo.o ` wouldn't yield "duplicate symbols" error anymore?
> > Yea I mentioned this with a top level comment as well, it does break the related test. I need to find a better heuristic for skipping these, since we only want to do it for LC_LINKER_OPTION's / duplicate -framework commands etc (not sure if the latter is handled today or not, especially as it conflicts with -framework foo -weak_framework foo etc).
> > 
> > I was hoping to do this specifically in the LC_LINKER_OPTION logic to avoid changing things here, but the issue is the order of hitting those options vs the command line options depends on the order things are passed, so I couldn't rely on that.
> Ah, I've missed the previous comments. Sorry!
> 
> In any case, I can think of a contrived example where you have the input obj files (standalone or in archives) containing no symbols but contributing to global ctors. In that case, wouldn't we want the side effect of these being loaded individually? 
> 
> 
I don't think it'd be terribly hard to handle this, we'd just need an extra boolean to `addFile` to indicate if a file is being loaded via `add{Framework,Library}` or if it's being loaded directly via a path. That said... I'm not sure there's much value in handling this edge case. I'm more than happy to just dedup everything and document this behavioral difference in ld64-vs-lld.rst.

> I can think of a contrived example where you have the input obj files (standalone or in archives) containing no symbols but contributing to global ctors

What does 'contributing to global ctors' mean exactly?


================
Comment at: lld/test/MachO/framework-object.ll:1
+; REQUIRES: x86
+; RUN: rm -rf %t; split-file %s %t
----------------
not sure this test deserves its own file... seems like we could fold it into framework.s and lc-linker-option.ll


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114841