[PATCH] D103522: [lld/mac] Address review feedback and improve a comment

[Nico Weber via Phabricator via llvm-commits]

thakis created this revision.
thakis added a reviewer: lld-macho.
Herald added a reviewer: int3.
Herald added a reviewer: gkm.
Herald added a project: lld-macho.
thakis requested review of this revision.

I forgot to move the message() call around as requested in D103428 <https://reviews.llvm.org/D103428>
before committing that change. Move it now.

Also, improve the ordinal uniq'ing comment. I hadn't realized that the
distinct-but-identical files happen with --reproduce and not in general.

No behavior change.


https://reviews.llvm.org/D103522

Files:
  lld/MachO/DriverUtils.cpp
  lld/MachO/InputFiles.cpp
  lld/MachO/Writer.cpp


Index: lld/MachO/Writer.cpp
===================================================================
--- lld/MachO/Writer.cpp
+++ lld/MachO/Writer.cpp
@@ -705,17 +705,21 @@
       // Several DylibFiles can have the same installName. Only emit a single
       // load command for that installName and give all these DylibFiles the
       // same ordinal.
-      // This can happen if:
+      // This can happen in several cases:
       // - a new framework could change its installName to an older
       //   framework name via an $ld$ symbol depending on platform_version
-      // - symlink (eg libpthread.tbd is a symlink to libSystem.tbd)
+      // - symlinks (for example, libpthread.tbd is a symlink to libSystem.tbd;
+      //   Foo.framework/Foo.tbd is usually a symlink to
+      //   Foo.framework/Versions/Current/Foo.tbd, where
+      //   Foo.framework/Versions/Current is usually a sylink to
+      //   Foo.framework/Versions/A)
       // - a framework can be linked both explicitly on the linker
       //   command line and implicitly as a reexport from a different
       //   framework. The re-export will usually point to the tbd file
       //   in Foo.framework/Versions/A/Foo.tbd, while the explicit link will
-      //   usually find Foo.framwork/Foo.tbd. These are usually two identical
-      //   but distinct files (concrete example: CFNetwork.framework, reexported
-      //   from CoreServices.framework).
+      //   usually find Foo.framwork/Foo.tbd. These are usually symlinks,
+      //   but in a --reproduce archive they will be identical but distinct
+      //   files.
       // In the first case, *semantically distinct* DylibFiles will have the
       // same installName.
       int64_t &ordinal = ordinalForInstallName[dylibFile->dylibName];
Index: lld/MachO/InputFiles.cpp
===================================================================
--- lld/MachO/InputFiles.cpp
+++ lld/MachO/InputFiles.cpp
@@ -826,6 +826,9 @@
     return;
   }
 
+  if (config->printEachFile)
+    message(toString(this));
+
   deadStrippable = hdr->flags & MH_DEAD_STRIPPABLE_DYLIB;
 
   if (!checkCompatibility(this))
@@ -900,6 +903,9 @@
   compatibilityVersion = interface.getCompatibilityVersion().rawValue();
   currentVersion = interface.getCurrentVersion().rawValue();
 
+  if (config->printEachFile)
+    message(toString(this));
+
   if (!is_contained(skipPlatformChecks, dylibName) &&
       !is_contained(interface.targets(), config->platformInfo.target)) {
     error(toString(this) + " is incompatible with " +
Index: lld/MachO/DriverUtils.cpp
===================================================================
--- lld/MachO/DriverUtils.cpp
+++ lld/MachO/DriverUtils.cpp
@@ -214,8 +214,6 @@
       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`
@@ -231,8 +229,6 @@
            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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D103522.349254.patch
Type: text/x-patch
Size: 3436 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210602/7ff2ad0d/attachment.bin>