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

Sat Sep 17 08:36:45 PDT 2022

BertalanD added inline comments.

================
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:
> BertalanD wrote:
> > 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.
> Software is one of the few areas where human brains have to deal with 9+ orders of magnitude within a relatively short timespan: a single bit can be a critical source of a bug, and right after that you might think about how to best organize gigabytes of data. You might try to shave off cycles form a very hot inner loop, or have a function that's 10^6 times as inefficient than it could be and it'll still finished imperceptibly fast as it's only called once.
> 
> This is really hard to wrap your brain around.
> 
> I get it, superfluous allocations feel silly. But a _single_ superfluous allocation that happens just one during startup is absolutely, completely irrelevant. (This one happens to go away by itself when we adopt C++20 too, but that fact really doesn't matter here.)
> 
> Go ahead and change it if you want (in a separate commit), but learning to quiet the part of the brain that screams and shouts "this! is! so! inefficient!" and considering if it actually matters is imho a very useful skill to have.
> 
> (In contrast, increasing readability and clarity code on the other hand is almost always worth it.)
This is great advice, I'm going to keep this in mind the next time I do changes like this one.

(fwiw, this change has already landed in 4f688d00f43056a94869aff32d5cdcddedf88a84 though)

================
Comment at: lld/MachO/SyntheticSections.cpp:2101
+  return alignTo<8>(sizeof(dyld_chained_starts_in_segment) +
+                    pageStarts.back().first * sizeof(uint16_t));
+}
----------------
thakis wrote:
> Since we always write this section: Can we end up in a situation where we write this section but `pageStarts` is empty (and calling `back()` is UB)?
> 
> AFAICT pageStarts is only added to if there actually are fixups, and a tiny program not calling anything might not have any (?)
`SegmentInfo` objects are created by traversing `ChainedFixupsSection::locations`, so they should always have at least one entry in `pageStarts`.

================
Comment at: lld/MachO/SyntheticSections.cpp:2109
+  segInfo->page_size = target->getPageSize();
+  // FIXME: Use DYLD_CHAINED_PTR_64_OFFSET on newer OS versions.
+  segInfo->pointer_format = DYLD_CHAINED_PTR_64;
----------------
thakis wrote:
> Out of curiosity: What are the implications of (not) doing this?
I do not know, unfortunately I couldn't find any design docs that would explain the rationale.

The source releases aren't too helpful either: ld64 mentions `// move to DYLD_CHAINED_PTR_64_OFFSET when OS is ready` (https://github.com/apple-opensource/ld64/blob/e28c028b20af187a16a7161d89e91868a450cadc/src/ld/OutputFile.cpp#L5249), but not much more.  

================
Comment at: lld/MachO/SyntheticSections.h:275
+//
+// Symbols are always bound eagerly when chained fixups are used. In that case,
+// StubSection contains indirect jumps to addresses stored in the GotSection.
----------------
thakis wrote:
> No change needed here, but: They're bound eagerly at page-in time on new OS versions, which in a way means they're always bound lazily as-needed, right?
> 
> (Comment is fine as-is, I'm asking just to make sure I'm understanding things. And there are OS versions that have chained fixups but no page-in linking.)
> They're bound eagerly at page-in time on new OS versions, which in a way means they're always bound lazily as-needed, right?

That's correct. They are bound lazily on macOS 13+ in the sense that symbol lookups aren't performed upfront at load time. I wouldn't call it true lazy binding though, as the observable behavior is the same as eager binding: symbols from `dlopen()`-ed dylibs can't fulfill undefined references:

```
❯ cat undef.c
int undef() { return 42; }
❯ clang -dynamiclib undef.c -o libundef.dylib
❯ cat main.c
#include <dlfcn.h>
int undef();

int main() { dlopen("./libundef.dylib", RTLD_NOW); return undef(); }
❯ clang main.c -undefined dynamic_lookup
ld: warning: -undefined dynamic_lookup may not work with chained fixups
❯ ./a.out
dyld[59352]: symbol not found in flat namespace '_undef'
[1]    59352 abort      ./a.out
```

I believe this is not only because the GOT entries for `undef` and `dlopen` end up on the same page (so calling `dlopen` would cause both symbols to be bound): the WWDC video mentions that `dlopen()`-ed dylibs continue to be relocated by `dyld`, so it wouldn't surprise me if the kernel knew nothing about their symbols at all even after they've been loaded.

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

https://reviews.llvm.org/D132560