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

[Keith Smiley via Phabricator via llvm-commits]

keith added inline comments.


================
Comment at: lld/MachO/Driver.cpp:391
 
+static DenseSet<StringRef> loadedContainerFrameworks;
 static void addFramework(StringRef name, bool isNeeded, bool isWeak,
----------------
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


================
Comment at: lld/MachO/Driver.cpp:402
+    file_magic magic = identify_magic((*buffer).getBuffer());
+    switch (magic) {
+    case file_magic::macho_object:
----------------
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


================
Comment at: lld/MachO/Driver.cpp:402
+    file_magic magic = identify_magic((*buffer).getBuffer());
+    switch (magic) {
+    case file_magic::macho_object:
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114841