[MACH-O] Fix the ASM code generated for __stub_helpers section

[ltr_]

Summary

I discovered that lld for darwin is generating the wrong code for lazy bindings in the __stub_helper section (at least for osx 10.12). This is the way i can reproduce this problem, using this program:

#include <stdio.h>

int main(int argc, char **argv) {
    printf("C: printf!\n");
    puts("C: puts!\n");
    return 0;
}

Then I link it using i have tested it in 3.9, 4.0 and 4.1 versions:

$ clang -c hello.c
$ lld -flavor darwin hello.o -o h1  -lc

When i execute the binary h1 the system gives me the following error:

C: printf!
dyld: lazy symbol binding failed: BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB
has segment 4 which is too large (0..3)
dyld: BIND_OPCODE_SET_SEGMENT_AND_OFFSET_ULEB has segment 4 which is too
large (0..3)
Trace/BPT trap: 5

Investigating the code, it seems that the problem is that the asm code generated in the file StubPass.cpp, specifically in the line 323,when it adds, what it seems an arbitrary number (12) to the offset into the lazy bind opcodes section, but it should be calculated depending on the MachONormalizedFileBinaryWrite::lazyBindingInfo result.

I confirmed this bug by patching the code manually in the binary and writing the right offset in the asm code (__stub_helper).

This patch fixes the content of the atom that contains the assembly code when the offset is known.

[hans]

Rui: can you help find a reviewer for this? It was requested for 5.0.

[ruiu]

I'm not familiar with the code and the behavior of the macOS loader. Lang, could you review?

[hans]

In D35387#822016, @ruiu wrote:

I'm not familiar with the code and the behavior of the macOS loader. Lang, could you review?

Lang: ping?

Rui: in case Lang is ooo, do you know anyone else who might be able to review this?

[ruiu]

It is probably me, but I don't believe this is a new bug in 5.0.

[lhames]

Sorry - looking in to this now...

[lhames]

I've made a couple of notes, and it would be great to get a test-case for this too, but otherwise it's looking good. Thanks so much for working on this!

lib/ReaderWriter/MachO/ArchHandler_arm.cpp

70–73 ↗

(On Diff #106543)

Given that we're returning lazyImmediateLocation on all platforms, can we just use this as the default implementation in ArchHandler::lazyImmediateLocationKind()?

lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp

1489 ↗

(On Diff #106543)

It would be good to add a comment for this function to explain what it's doing.

1503–1514 ↗

(On Diff #106543)

It would be good to create the target section outside the loop so that we can apply all patches in one go -- right now this looks like it will eat up a lot of memory maintaining partially-patched copies of the section that are no longer referenced.

[ltr_]

lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp

1503–1514 ↗

(On Diff #106543)

Ill do this tomorrow.
+ the comments for this function

[andrewrk]

Bump.

• Can we have this even without tests?
• Can we have this in 5.0.0?

[ruiu]

Lang,

What do you think of this patch? If you think this is okay to commit and should be backported to 5.0, you might want to do that, since 5.0 release is happening really soon.

If you are busy, you can just give us a green light, so that I (or maybe Andrew Kelley) can do that for you.

[lhames]

It's probably better to have without tests than to not have at all.

ltr_ - Do you have commit access?

[ltr_]

No i do not have commit access. anyways Ill be unable to do a commit right now until the weekend =/

[hans]

In D35387#849537, @ruiu wrote:

Lang,

What do you think of this patch? If you think this is okay to commit and should be backported to 5.0, you might want to do that, since 5.0 release is happening really soon.

If you are busy, you can just give us a green light, so that I (or maybe Andrew Kelley) can do that for you.

It sounds like Lang ok'd it, and ltr_ isn't around to commit at the moment.

Rui, would you mind committing this so we get it into the release?

lld/trunk/lib/ReaderWriter/MachO/ArchHandler.h

public:
   /// Used by normalizedFromAtoms() to know where to generated rebasing and
   /// binding info in final executables.
   virtual bool isPointer(const Reference &) = 0;
 
   /// Used by normalizedFromAtoms() to know where to generated lazy binding
   /// info in final executables.
   virtual bool isLazyPointer(const Reference &);
 
+  /// Reference from an __stub_helper entry to the required offset of the
+  /// lazy bind commands.
+  virtual Reference::KindValue lazyImmediateLocationKind() = 0;
+
   /// Returns true if the specified relocation is paired to the next relocation.
   virtual bool isPairedReloc(const normalized::Relocation &) = 0;
 
   /// Prototype for a helper function.  Given a sectionIndex and address,
   /// finds the atom and offset with that atom of that address.
   typedef std::function<llvm::Error (uint32_t sectionIndex, uint64_t addr,
                         const lld::Atom **, Reference::Addend *)>
                         FindAtomBySectionAndAddress;

lld/trunk/lib/ReaderWriter/MachO/ArchHandler_arm.cpp

public:
   Reference::KindValue unwindRefToFunctionKind() override {
     return invalid;
   }
 
   Reference::KindValue unwindRefToEhFrameKind() override {
     return invalid;
   }
 
+  Reference::KindValue lazyImmediateLocationKind() override {
+    return lazyImmediateLocation;
+  }
+
   Reference::KindValue pointerKind() override {
     return invalid;
   }
 
   uint32_t dwarfCompactUnwindType() override {
     // FIXME
     return -1;
   }

lld/trunk/lib/ReaderWriter/MachO/ArchHandler_arm64.cpp

public:
   Reference::KindValue unwindRefToEhFrameKind() override {
     return unwindInfoToEhFrame;
   }
 
   Reference::KindValue pointerKind() override {
     return pointer64;
   }
 
+  Reference::KindValue lazyImmediateLocationKind() override {
+    return lazyImmediateLocation;
+  }
+
   uint32_t dwarfCompactUnwindType() override {
     return 0x03000000;
   }
 
   llvm::Error getReferenceInfo(const normalized::Relocation &reloc,
                                const DefinedAtom *inAtom,
                                uint32_t offsetInAtom,
                                uint64_t fixupAddress, bool isBig,

lld/trunk/lib/ReaderWriter/MachO/ArchHandler_x86.cpp

public:
   Reference::KindValue unwindRefToCIEKind() override {
     return negDelta32;
   }
 
   Reference::KindValue unwindRefToFunctionKind() override{
     return delta32;
   }
 
+  Reference::KindValue lazyImmediateLocationKind() override {
+    return lazyImmediateLocation;
+  }
+
   Reference::KindValue unwindRefToEhFrameKind() override {
     return invalid;
   }
 
   Reference::KindValue pointerKind() override {
     return invalid;
   }
 

lld/trunk/lib/ReaderWriter/MachO/ArchHandler_x86_64.cpp

public:
   Reference::KindValue unwindRefToCIEKind() override {
     return negDelta32;
   }
 
   Reference::KindValue unwindRefToFunctionKind() override{
     return unwindFDEToFunction;
   }
 
+  Reference::KindValue lazyImmediateLocationKind() override {
+    return lazyImmediateLocation;
+  }
+
   Reference::KindValue unwindRefToEhFrameKind() override {
     return unwindInfoToEhFrame;
   }
 
   Reference::KindValue pointerKind() override {
     return pointer64;
   }
 

lld/trunk/lib/ReaderWriter/MachO/MachONormalizedFileFromAtoms.cpp

void         segIndexForSection(const SectionInfo *sect,
                              uint8_t &segmentIndex, uint64_t &segmentStartAddr);
   const Atom  *targetOfLazyPointer(const DefinedAtom *lpAtom);
   const Atom  *targetOfStub(const DefinedAtom *stubAtom);
   llvm::Error getSymbolTableRegion(const DefinedAtom* atom,
                                    bool &inGlobalsRegion,
                                    SymbolScope &symbolScope);
   void         appendSection(SectionInfo *si, NormalizedFile &file);
   uint32_t     sectionIndexForAtom(const Atom *atom);
+  void fixLazyReferenceImm(const DefinedAtom *atom, uint32_t offset,
+                           NormalizedFile &file);
 
   typedef llvm::DenseMap<const Atom*, uint32_t> AtomToIndex;
   struct AtomAndIndex { const Atom *atom; uint32_t index; SymbolScope scope; };
   struct AtomSorter {
     bool operator()(const AtomAndIndex &left, const AtomAndIndex &right);
   };
   struct SegmentSorter {
     bool operator()(const SegmentInfo *left, const SegmentInfo *right);

 
 void Util::addRebaseAndBindingInfo(const lld::File &atomFile,
                                                         NormalizedFile &nFile) {
   if (_ctx.outputMachOType() == llvm::MachO::MH_OBJECT)
     return;
 
   uint8_t segmentIndex;
   uint64_t segmentStartAddr;
+  uint32_t offsetInBindInfo = 0;
+
   for (SectionInfo *sect : _sectionInfos) {
     segIndexForSection(sect, segmentIndex, segmentStartAddr);
     for (const AtomInfo &info : sect->atomsAndOffsets) {
       const DefinedAtom *atom = info.atom;
       for (const Reference *ref : *atom) {
         uint64_t segmentOffset = _atomToAddress[atom] + ref->offsetInAtom()
                                 - segmentStartAddr;
         const Atom* targ = ref->target();
for (const AtomInfo &info : sect->atomsAndOffsets) {
           }
           bind.segIndex = segmentIndex;
           bind.segOffset = segmentOffset;
           bind.kind = llvm::MachO::BIND_TYPE_POINTER;
           bind.canBeNull = false; //sa->canBeNullAtRuntime();
           bind.symbolName = targ->name();
           bind.addend = ref->addend();
           nFile.lazyBindingInfo.push_back(bind);
+
+          // Now that we know the segmentOffset and the ordinal attribute,
+          // we can fix the helper's code
+
+          fixLazyReferenceImm(atom, offsetInBindInfo, nFile);
+
+          // 5 bytes for opcodes + variable sizes (target name + \0 and offset
+          // encode's size)
+          offsetInBindInfo +=
+              6 + targ->name().size() + llvm::getULEB128Size(bind.segOffset);
+          if (bind.ordinal > BIND_IMMEDIATE_MASK)
+            offsetInBindInfo += llvm::getULEB128Size(bind.ordinal);
+        }
+      }
+    }
+  }
+}
+
+void Util::fixLazyReferenceImm(const DefinedAtom *atom, uint32_t offset,
+                               NormalizedFile &file) {
+  for (const auto &ref : *atom) {
+    const DefinedAtom *da = dyn_cast<DefinedAtom>(ref->target());
+    if (da == nullptr)
+      return;
+
+    const Reference *helperRef = nullptr;
+    for (const Reference *hr : *da) {
+      if (hr->kindValue() == _archHandler.lazyImmediateLocationKind()) {
+        helperRef = hr;
+        break;
+      }
+    }
+    if (helperRef == nullptr)
+      continue;
+
+    // TODO: maybe get the fixed atom content from _archHandler ?
+    for (SectionInfo *sectInfo : _sectionInfos) {
+      for (const AtomInfo &atomInfo : sectInfo->atomsAndOffsets) {
+        if (atomInfo.atom == helperRef->target()) {
+          auto sectionContent =
+              file.sections[sectInfo->normalizedSectionIndex].content;
+          uint8_t *rawb =
+              file.ownedAllocations.Allocate<uint8_t>(sectionContent.size());
+          llvm::MutableArrayRef<uint8_t> newContent{rawb,
+                                                    sectionContent.size()};
+          std::copy(sectionContent.begin(), sectionContent.end(),
+                    newContent.begin());
+          llvm::support::ulittle32_t *loc =
+              reinterpret_cast<llvm::support::ulittle32_t *>(
+                  &newContent[atomInfo.offsetInSection +
+                              helperRef->offsetInAtom()]);
+          *loc = offset;
+          file.sections[sectInfo->normalizedSectionIndex].content = newContent;
         }
       }
     }
   }
 }
 
 void Util::addExportInfo(const lld::File &atomFile, NormalizedFile &nFile) {
   if (_ctx.outputMachOType() == llvm::MachO::MH_OBJECT)

lld/trunk/test/mach-o/lazy-bind-x86_64.yaml

 
 # CHECK-NM:   (undefined) external _bar (from libbar)
 # CHECK-NM:   (undefined) external _baz (from libbaz)
 # CHECK-NM:   (undefined) external _foo (from libfoo)
 
 
 # CHECK-HELPERS:Disassembly of section __TEXT,__stub_helper:
 # CHECK-HELPERS:  68 00 00 00 00            pushq $0
-# CHECK-HELPERS:  68 10 00 00 00            pushq $16
-# CHECK-HELPERS:  68 20 00 00 00            pushq $32
+# CHECK-HELPERS:  68 0b 00 00 00            pushq $11
+# CHECK-HELPERS:  68 16 00 00 00            pushq $22
 
 # Make sure the stub helper is correctly aligned
 # CHECK-DYLIBS:   sectname __stub_helper
 # CHECK-DYLIBS-NEXT:    segname __TEXT
 # CHECK-DYLIBS-NEXT:       addr
 # CHECK-DYLIBS-NEXT:       size
 # CHECK-DYLIBS-NEXT:     offset
 # CHECK-DYLIBS-NEXT:      align 2^2 (4)