[lld/mac] Make -t work correctly with -flat_namespace

[thakis]

Summary

We used to not print dylibs referenced by other dylibs in -t mode. This
affected reexports, and with -flat_namespace also just dylibs loaded by
dylibs. Now we print them.

Fixes PR49514.

[int3]

Hm, could we not just emit the message inside the DylibFile and ObjFile constructors?

Also nit: Could we have more detail in the commit message? Referencing bugzilla is a bit inconvenient (plus it could get confusing if more comments are left on the issue.)

Finally: There are a bunch of things listed in that task as "interesting things to check", not sure if we want to have more tests for those, but your call

lld/test/MachO/flat-namespace.s

19

nit: align with -NEXT: lines

[thakis]

In D103428#2791652, @int3 wrote:

Hm, could we not just emit the message inside the DylibFile and ObjFile constructors?

Yes, putting it in the two DylibFile ctors works just as well. It's the same +4 lines, and printing things from a ctor feels a bit weird, so I put it where I put it but if you want I can change it.

Also nit: Could we have more detail in the commit message? Referencing bugzilla is a bit inconvenient (plus it could get confusing if more comments are left on the issue.)

Sure, adding more details.

Finally: There are a bunch of things listed in that task as "interesting things to check", not sure if we want to have more tests for those, but your call

Yes, I should add more tests for some of these things, but I think they already work :)

[thakis]

tweak test

[int3]

I'd prefer we put in in the ctors: I agree that having side-effects in a constructor is a bit weird, but OTOH it means that we'll never have to worry about new instantiation sites forgetting to add this logging.

[thakis]

In D103428#2791973, @int3 wrote:

I'd prefer we put in in the ctors: I agree that having side-effects in a constructor is a bit weird, but OTOH it means that we'll never have to worry about new instantiation sites forgetting to add this logging.

I just realized I landed this without addressing this comment. Sorry, that wasn't intentional, I just forgot. I'll fix it up in a follow-up commit.

lld/MachO/Driver.cpp

if (DylibFile *dylibFile = loadDylib(mbref, nullptr, isBundleLoader))
       newFile = dylibFile;
     break;
   default:
     error(path + ": unhandled file type");
   }
   if (newFile) {
     // printArchiveMemberLoad() prints both .a and .o names, so no need to
     // print the .a name here.
-    if (config->printEachFile && magic != file_magic::archive)
+    if (config->printEachFile && magic != file_magic::archive &&
+        !isa<DylibFile>(newFile))
       message(toString(newFile));
     inputFiles.insert(newFile);
   }
   return newFile;
 }
 
 static void addLibrary(StringRef name, bool isWeak, bool isExplicit = true) {
   if (Optional<StringRef> path = findLibrary(name)) {

lld/MachO/DriverUtils.cpp

DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
   if (magic == file_magic::tapi_file) {
     Expected<std::unique_ptr<InterfaceFile>> result = TextAPIReader::get(mbref);
     if (!result) {
       error("could not load TAPI file at " + mbref.getBufferIdentifier() +
             ": " + toString(result.takeError()));
       return nullptr;
     }
     file = make<DylibFile>(**result, umbrella, isBundleLoader);
+    if (config->printEachFile)
+      message(toString(file));
 
     // parseReexports() can recursively call loadDylib(). That's fine since
     // we wrote DylibFile we just loaded to the loadDylib cache via the `file`
     // reference. But the recursive load can grow loadDylibs, so the `file`
     // reference might become invalid after parseReexports() -- so copy the
     // pointer it refers to before going on.
     newFile = file;
     if (newFile->exportingFile)
       newFile->parseReexports(**result);
   } else {
     assert(magic == file_magic::macho_dynamically_linked_shared_lib ||
            magic == file_magic::macho_dynamically_linked_shared_lib_stub ||
            magic == file_magic::macho_executable ||
            magic == file_magic::macho_bundle);
     file = make<DylibFile>(mbref, umbrella, isBundleLoader);
+    if (config->printEachFile)
+      message(toString(file));
 
     // parseLoadCommands() can also recursively call loadDylib(). See comment
     // in previous block for why this means we must copy `file` here.
     newFile = file;
     if (newFile->exportingFile)
       newFile->parseLoadCommands(mbref, umbrella);
   }
   return newFile;

lld/test/MachO/flat-namespace.s

 # REQUIRES: x86
 # RUN: rm -rf %t; split-file %s %t
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t/foo.o %t/foo.s
 # RUN: %lld -dylib -o %t/foo.dylib %t/foo.o
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t/bar.o %t/bar.s
 # RUN: %lld -lSystem -dylib -o %t/bar.dylib %t/bar.o %t/foo.dylib
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t/baz.o %t/baz.s
 # RUN: %lld -lSystem -dylib -o %t/baz.dylib %t/baz.o %t/bar.dylib
 
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t/main.o %t/main.s
 
 # With flat_namespace, the linker automatically looks in foo.dylib and
 # bar.dylib too, but it doesn't add a LC_LOAD_DYLIB for it.
-# RUN: %lld -flat_namespace -lSystem %t/main.o %t/baz.dylib -o %t/out
+# RUN: %lld -flat_namespace -lSystem %t/main.o %t/baz.dylib -o %t/out -t | \
+# RUN:   FileCheck --check-prefix=T %s
+## FIXME: The `bar.dylib` line should use `T-NEXT`, but on Windows we load
+## libSystem.tbd with different slash styles and end up loading it twice
+## for that reason.
+# T:      main.o
+# T-NEXT: baz.dylib
+# T:      bar.dylib
+# T-NEXT: foo.dylib
 # RUN: llvm-objdump --macho --all-headers %t/out \
 # RUN:     | FileCheck --check-prefix=HEADERBITS %s
 # RUN: llvm-objdump --macho --bind --lazy-bind --weak-bind %t/out \
 # RUN:     | FileCheck --check-prefix=FLAT %s
 # RUN: llvm-nm -m %t/out | FileCheck --check-prefix=FLATSYM %s
 # RUN: llvm-readobj --syms %t/out | FileCheck --check-prefix=FLATSYM-READOBJ %s
 
 # HEADERBITS-NOT: NOUNDEFS