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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.

lgtm, thanks for taking the time to hash out the different approaches!



================
Comment at: lld/MachO/Driver.cpp:391
 
+static DenseSet<StringRef> loadedContainerFrameworks;
 static void addFramework(StringRef name, bool isNeeded, bool isWeak,
----------------
keith wrote:
> int3 wrote:
> > Q: why the name "container"?
> I don't feel strongly about it, totally up for suggestions, my thought here was that unlike dylib frameworks where the framework structure is long lived, for these other file types the framework structure just serves as an ephemeral container for the binary that's then linked in and the framework is practically discarded
hmm. that's... quite a few steps of thinking :D

how about `loadedObjectFrameworks`?


================
Comment at: lld/MachO/Driver.cpp:402
+    file_magic magic = identify_magic((*buffer).getBuffer());
+    switch (magic) {
+    case file_magic::macho_object:
----------------
keith wrote:
> keith wrote:
> > oontvoo wrote:
> > > int3 wrote:
> > > > is this cleaner than caching all frameworks, regardless of file type? Or would we actually behave differently if this cached all files?
> > > > 
> > > > (just curious, not asking you to change it. But we should probably have a comment here about why we're only caching object and bitcode files)
> > > Similar topic, is it better to check the cache first? (before even bothering to read the content of the file?)
> > > is this cleaner than caching all frameworks, regardless of file type? Or would we actually behave differently if this cached all files?
> > > 
> > > (just curious, not asking you to change it. But we should probably have a comment here about why we're only caching object and bitcode files)
> > 
> > I was careful to exclude dylibs from this because theoretically their `isNeeded` and other attrs could flip (I'm not sure how gracefully that's handled today), and I assume in that case we want to be careful about letting new loads overwrite previous loads? For the other file types I was just trying to reduce the radius of this logic, since archives for example have separate caching logic in addFile
> > Similar topic, is it better to check the cache first? (before even bothering to read the content of the file?)
> 
> Great point! Moving the cache checking up here greatly simplifies the implementation, since we no longer have to do the magic reading at all this way! Which seems obvious in retrospect
sgtm. Can we have a comment here that mentions how dylib and archives and separate handling?


================
Comment at: lld/MachO/Driver.cpp:425
     }
+    if (cacheLoad && file) {
+      loadedContainerFrameworks.insert(*path);
----------------
int3 wrote:
> nit: no need for braces
so actually we avoid braces when it's a single-line `if` statement, but if we are looking at `else if`, the convention is to use braces for it if any other part of the if-else chain requires braces. Sorry to make you go back and forth...


================
Comment at: lld/test/MachO/lc-linker-option.ll:85
+; RUN: llc --filetype=obj %t/load-framework-twice.ll -o %t/main
+; Order of the object with the LC_LINKER_OPTION vs -framework arg is important.
+; RUN: %lld %t/main -F %t -framework Foo -framework Foo -o /dev/null
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114841