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

[Daniel Bertalan via Phabricator via llvm-commits]

BertalanD marked an inline comment as not done.
BertalanD added a comment.

Thank you for the review!



================
Comment at: lld/MachO/Driver.cpp:933
+        {PLATFORM_WATCHOS, VersionTuple(6, 0)},
+        {PLATFORM_BRIDGEOS, VersionTuple(4, 0)}}};
   PlatformType platform = removeSimulator(config->platformInfo.target.Platform);
----------------
thakis wrote:
> (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.)
I know this change is absolutely insignificant, but this pointless allocation has been bothering me so much...

I'll land this change separately.


================
Comment at: lld/MachO/Driver.cpp:982
+           config->platformInfo.minimum.getAsString());
+      return true;
+    }
----------------
thakis wrote:
> 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.
> Shouldn't this return false too (…or fall through to the return false two lines further down)? 

Oh, right. It should definitely fall through. I didn't want to make this a hard error, because ld64 doesn't have such  a diagnostic at all. I figured we might want to play around with enabling it for unsupported targets to see what happens, and calling `error()` would have prevented that.

As for the rest of the diagnostics in this function:
- Chained fixups do exist for arm64_32, but this patch does not attempt to implement the 32-bit encoding, which has some peculiarities due to the `next` offsets not being able to encode the whole range of a page. And I don't think we should make things more complicated for the sake of a probably soon-to-be-obsoleted platform.
- ld64 falls back to generating dyld opcodes when PIE is disabled, even if chained fixups were explicitly requested with `-fixup_chains`. That's just weird.

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

Agreed, I'm on it.


================
Comment at: lld/MachO/Options.td:1231
+    HelpText<"Emit chained fixups (experimental)">,
+    Group<grp_undocumented>;
+def no_fixup_chains : Flag<["-"], "no_fixup_chains">,
----------------
thakis wrote:
> 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.)
I'll move it into `grp_rare` then. `-fixup_chains` is not documented in the ld64 man page, but I guess it's useful enough to move it to be near the less esoteric options. 


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

https://reviews.llvm.org/D132560