[PATCH] D132560: [lld-macho] Add initial support for chained fixups

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Sorry that the review is taking so long! Here's one question about the driver code (and a few minor suggestions).



================
Comment at: lld/MachO/Driver.cpp:933
+        {PLATFORM_WATCHOS, VersionTuple(6, 0)},
+        {PLATFORM_BRIDGEOS, VersionTuple(4, 0)}}};
   PlatformType platform = removeSimulator(config->platformInfo.target.Platform);
----------------
(This seems unrelated. I'd land that separately if you want (no review needed), or just revert it. This runs just once, so std::vector is good enough, and in c++20 vector is constexpr anyways. And having to manually repeat the number of elements is a tiny bit inconvenient.)


================
Comment at: lld/MachO/Driver.cpp:982
+           config->platformInfo.minimum.getAsString());
+      return true;
+    }
----------------
Shouldn't this `return false` too (…or fall through to the return false two lines further down)? (If this is intentional, then this check should probably be below the arch and pic checks below – else we return true here without. checkout archs and pic!)

Does ld just warn when this happens? Maybe we should see if we can get away with making this an error.

Maybe it'd be good to have some tests for the diags here.


================
Comment at: lld/MachO/Driver.cpp:1666
-                     args.hasFlag(OPT_pie, OPT_no_pie, true));
-
     // Now that all dylibs have been loaded, search for those that should be
----------------
If you want, you can land moving this further up in a separate commit too (
"""
[lld/mac] Move isPic up

Needed for chainedFixups patch.
No behavior change.
""") – just land, no review needed.

But keeping it here is fine too.


================
Comment at: lld/MachO/Options.td:1231
+    HelpText<"Emit chained fixups (experimental)">,
+    Group<grp_undocumented>;
+def no_fixup_chains : Flag<["-"], "no_fixup_chains">,
----------------
I think you can move these out of undocumented. The help text says "experimental", that seems clear enough :)

(…and then we won't forget to move it out of undocumented when we enable it by default.)


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

https://reviews.llvm.org/D132560