[lld] [PATCH] Properly handle relocation for LSDA in MachO eh-frames.
Lang Hames
lhames at gmail.com
Tue May 12 14:53:58 PDT 2015
Hi Rui,
> Do you mind if I ask you to upload the patch to Fabricator? It'd much
easier to review.
No problem. I've uploaded the patch, with fixes to address your initial
comments, in http://reviews.llvm.org/D9723 .
Cheers,
Lang.
On Tue, May 12, 2015 at 1:00 PM, Rui Ueyama <ruiu at google.com> wrote:
> Do you mind if I ask you to upload the patch to Fabricator? It'd much
> easier to review.
>
> > Index: lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp
> > ===================================================================
> > --- lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp (revision
> 237164)
> > +++ lib/ReaderWriter/MachO/MachONormalizedFileToAtoms.cpp (working copy)
> > @@ -29,6 +29,7 @@
> > #include "lld/Core/LLVM.h"
> > #include "llvm/Support/Format.h"
> > #include "llvm/Support/MachO.h"
> > +#include "llvm/Support/LEB128.h"
> >
> > using namespace llvm::MachO;
> > using namespace lld::mach_o::normalized;
> > @@ -644,11 +645,151 @@
> > return res;
> > }
> >
> > +/// --- Augmentation String Processing ---
> > +
> > +class AugmentationDataInfo {
> > +public:
> > + AugmentationDataInfo()
> > + : present(false), mayHaveLSDA(false) {}
> > +
> > + bool present;
> > + bool mayHaveLSDA;
> > +};
>
> You can use C++ non-static member initialization here.
> Also please follow the LLD coding style (I know that having
> different coding style is not a good idea, but this is how
> the code is formatted for LLD).
>
> bool _present = false;
> bool _mayHaveLSDA = false;
>
> > +static std::error_code processAugmentationString(const uint8_t *augStr,
> > + AugmentationDataInfo
> &adi,
> > + unsigned *len =
> nullptr) {
> > +
> > + if (augStr[0] == '\0') {
> > + if (len)
> > + *len = 1;
> > + return std::error_code();
> > + }
> > +
> > + if (augStr[0] != 'z')
> > + return make_dynamic_error_code("expected 'z' at start of
> augmentation "
> > + "string");
> > +
> > + adi.present = true;
> > + uint64_t Idx = 1;
>
> idx
>
> > + while (augStr[Idx] != '\0') {
> > + if (augStr[Idx] == 'L') {
> > + adi.mayHaveLSDA = true;
> > + ++Idx;
> > + } else
> > + ++Idx;
> > + }
> > +
> > + if (len)
> > + *len = Idx + 1;
> > + return std::error_code();
> > +}
> > +
> > +static std::error_code processCIE(const NormalizedFile &normalizedFile,
> > + MachODefinedAtom *atom,
> > + AugmentationDataInfo &adi) {
> > + const bool isBig =
> MachOLinkingContext::isBigEndian(normalizedFile.arch);
> > +
> > + const uint8_t *frameData = atom->rawContent().data();
> > + uint32_t size = read32(frameData, isBig);
> > + uint64_t cieIDField = size == 0xffffffffU
> > + ? sizeof(uint32_t) + sizeof(uint64_t)
> > + : sizeof(uint32_t);
>
> sizeof(uint32_t) or sizeof(uint64_t) look a bit odd to me because
> they are always 4 or 8.
>
> > + uint64_t versionField = cieIDField + sizeof(uint32_t);
> > + uint64_t augmentationStringField = versionField + sizeof(uint8_t);
> > +
> > + if (auto err = processAugmentationString(frameData +
> augmentationStringField,
> > + adi))
> > + return err;
> > +
> > + return std::error_code();
> > +}
> > +
> > +static std::error_code processFDE(const NormalizedFile &normalizedFile,
> > + MachOFile &file,
> > + mach_o::ArchHandler &handler,
> > + const Section *ehFrameSection,
> > + MachODefinedAtom *atom,
> > + uint64_t offset,
> > + const AugmentationDataInfo &adi) {
> > +
> > + const bool isBig =
> MachOLinkingContext::isBigEndian(normalizedFile.arch);
> > + const bool is64 = MachOLinkingContext::is64Bit(normalizedFile.arch);
> > +
> > + // Compiler wasn't lazy and actually told us what it meant.
> > + if (atom->begin() != atom->end())
> > + return std::error_code();
> > +
> > + const uint8_t *frameData = atom->rawContent().data();
> > + uint32_t size = read32(frameData, isBig);
> > + uint64_t cieFieldInFDE = size == 0xffffffffU
> > + ? sizeof(uint32_t) + sizeof(uint64_t)
> > + : sizeof(uint32_t);
> > +
> > + // Linker needs to fixup a reference from the FDE to its parent CIE (a
> > + // 32-bit byte offset backwards in the __eh_frame section).
> > + uint32_t cieDelta = read32(frameData + cieFieldInFDE, isBig);
> > + uint64_t cieAddress = ehFrameSection->address + offset +
> cieFieldInFDE;
> > + cieAddress -= cieDelta;
> > +
> > + Reference::Addend addend;
> > + const Atom *cie =
> > + findAtomCoveringAddress(normalizedFile, file, cieAddress, &addend);
> > + atom->addReference(cieFieldInFDE, handler.unwindRefToCIEKind(), cie,
> > + addend, handler.kindArch());
> > +
> > + // Linker needs to fixup reference from the FDE to the function it's
> > + // describing. FIXME: there are actually different ways to do this,
> and the
> > + // particular method used is specified in the CIE's augmentation
> fields
> > + // (hopefully)
> > + uint64_t rangeFieldInFDE = cieFieldInFDE + sizeof(uint32_t);
> > +
> > + int64_t functionFromFDE = readSPtr(is64, isBig,
> > + frameData + rangeFieldInFDE);
> > + uint64_t rangeStart = ehFrameSection->address + offset +
> rangeFieldInFDE;
> > + rangeStart += functionFromFDE;
> > +
> > + const Atom *func =
> > + findAtomCoveringAddress(normalizedFile, file, rangeStart, &addend);
> > + atom->addReference(rangeFieldInFDE, handler.unwindRefToFunctionKind(),
> > + func, addend, handler.kindArch());
> > +
> > + // Handle the augmentation data if there is any.
> > + if (adi.present) {
> > + // First process the augmentation data length field.
> > + uint64_t AugmentationDataLengthFieldInFDE =
> > + rangeFieldInFDE + 2 * (is64 ? sizeof(uint64_t) :
> sizeof(uint32_t));
> > + unsigned LengthFieldSize = 0;
> > + uint64_t AugmentationDataLength =
> > + llvm::decodeULEB128(frameData + AugmentationDataLengthFieldInFDE,
> > + &LengthFieldSize);
>
> lengthFieldSize
> ArgumentationDataLength
>
> > +
> > + if (adi.mayHaveLSDA && AugmentationDataLength > 0) {
> > +
> > + // Look at the augmentation data field.
> > + uint64_t AugmentationDataFieldInFDE =
> > + AugmentationDataLengthFieldInFDE + LengthFieldSize;
> > +
> > + int64_t lsdaFromFDE = readSPtr(is64, isBig,
> > + frameData +
> AugmentationDataFieldInFDE);
> > + uint64_t lsdaStart =
> > + ehFrameSection->address + offset + AugmentationDataFieldInFDE +
> > + lsdaFromFDE;
> > + const Atom *lsda =
> > + findAtomCoveringAddress(normalizedFile, file, lsdaStart,
> &addend);
> > + atom->addReference(AugmentationDataFieldInFDE,
> > + handler.unwindRefToFunctionKind(),
> > + lsda, addend, handler.kindArch());
> > + }
> > + }
> > +
> > + return std::error_code();
> > +}
> > +
> > std::error_code addEHFrameReferences(const NormalizedFile
> &normalizedFile,
> > MachOFile &file,
> > mach_o::ArchHandler &handler) {
> > - const bool isBig =
> MachOLinkingContext::isBigEndian(normalizedFile.arch);
> > - const bool is64 = MachOLinkingContext::is64Bit(normalizedFile.arch);
> >
> > const Section *ehFrameSection = nullptr;
> > for (auto §ion : normalizedFile.sections)
> > @@ -662,51 +803,27 @@
> > if (!ehFrameSection)
> > return std::error_code();
> >
> > + std::error_code ehFrameErr;
> > + AugmentationDataInfo adi;
> > +
> > file.eachAtomInSection(*ehFrameSection,
> > [&](MachODefinedAtom *atom, uint64_t offset)
> -> void {
> > assert(atom->contentType() == DefinedAtom::typeCFI);
> >
> > - if (ArchHandler::isDwarfCIE(isBig, atom))
> > + // Bail out if we've encountered an error.
> > + if (ehFrameErr)
> > return;
> >
> > - // Compiler wasn't lazy and actually told us what it meant.
> > - if (atom->begin() != atom->end())
> > - return;
> > + const bool isBig =
> MachOLinkingContext::isBigEndian(normalizedFile.arch);
> > + if (ArchHandler::isDwarfCIE(isBig, atom)) {
> > + adi = AugmentationDataInfo();
> > + ehFrameErr = processCIE(normalizedFile, atom, adi);
> > + } else
> > + ehFrameErr = processFDE(normalizedFile, file, handler,
> ehFrameSection,
> > + atom, offset, adi);
> > + });
> >
> > - const uint8_t *frameData = atom->rawContent().data();
> > - uint32_t size = read32(frameData, isBig);
> > - uint64_t cieFieldInFDE = size == 0xffffffffU
> > - ? sizeof(uint32_t) + sizeof(uint64_t)
> > - : sizeof(uint32_t);
> > -
> > - // Linker needs to fixup a reference from the FDE to its parent CIE
> (a
> > - // 32-bit byte offset backwards in the __eh_frame section).
> > - uint32_t cieDelta = read32(frameData + cieFieldInFDE, isBig);
> > - uint64_t cieAddress = ehFrameSection->address + offset +
> cieFieldInFDE;
> > - cieAddress -= cieDelta;
> > -
> > - Reference::Addend addend;
> > - const Atom *cie =
> > - findAtomCoveringAddress(normalizedFile, file, cieAddress,
> &addend);
> > - atom->addReference(cieFieldInFDE, handler.unwindRefToCIEKind(), cie,
> > - addend, handler.kindArch());
> > -
> > - // Linker needs to fixup reference from the FDE to the function it's
> > - // describing. FIXME: there are actually different ways to do this,
> and the
> > - // particular method used is specified in the CIE's augmentation
> fields
> > - // (hopefully)
> > - uint64_t rangeFieldInFDE = cieFieldInFDE + sizeof(uint32_t);
> > -
> > - int64_t functionFromFDE = readSPtr(is64, isBig, frameData +
> rangeFieldInFDE);
> > - uint64_t rangeStart = ehFrameSection->address + offset +
> rangeFieldInFDE;
> > - rangeStart += functionFromFDE;
> > -
> > - const Atom *func =
> > - findAtomCoveringAddress(normalizedFile, file, rangeStart,
> &addend);
> > - atom->addReference(rangeFieldInFDE,
> handler.unwindRefToFunctionKind(), func,
> > - addend, handler.kindArch());
> > - });
> > - return std::error_code();
> > + return ehFrameErr;
> > }
> >
> >
> > Index: test/mach-o/parse-eh-frame-relocs-x86_64.yaml
> > ===================================================================
> > --- test/mach-o/parse-eh-frame-relocs-x86_64.yaml (revision 0)
> > +++ test/mach-o/parse-eh-frame-relocs-x86_64.yaml (working copy)
> > @@ -0,0 +1,107 @@
> > +# RUN: lld -flavor darwin -arch x86_64 -r -print_atoms %s -o %t |
> FileCheck %s
> > +#
> > +# Test parsing of x86_64 __eh_frame (dwarf unwind) relocations.
> > +#
> > +#_catchMyException:
> > +# pushq %rbp
> > +# movq %rsp, %rbp
> > +# callq _foo
> > +# popq %rbp
> > +# retq
> > +# movq %rax, %rdi
> > +# callq ___cxa_begin_catch
> > +# popq %rbp
> > +# jmp ___cxa_end_catch
> > +
> > +--- !mach-o
> > +arch: x86_64
> > +file-type: MH_OBJECT
> > +flags: [ ]
> > +sections:
> > + - segment: __TEXT
> > + section: __text
> > + type: S_REGULAR
> > + attributes: [ S_ATTR_PURE_INSTRUCTIONS,
> S_ATTR_SOME_INSTRUCTIONS ]
> > + address: 0x0000000000000000
> > + content: [0x55, 0x48, 0x89, 0xe5, 0xe8, 0x00, 0x00, 0x00,
> > + 0x00, 0x5d, 0xc3, 0x48, 0x89, 0xc7, 0xe8, 0x00,
> > + 0x00, 0x00, 0x00, 0x5d, 0xe9, 0x00, 0x00, 0x00,
> > + 0x00, 0x00, 0x00, 0x00 ]
> > + relocations:
> > + - offset: 0x00000015
> > + type: X86_64_RELOC_BRANCH
> > + length: 2
> > + pc-rel: true
> > + extern: true
> > + symbol: 2
> > + - offset: 0x0000000f
> > + type: X86_64_RELOC_BRANCH
> > + length: 2
> > + pc-rel: true
> > + extern: true
> > + symbol: 1
> > + - offset: 0x00000005
> > + type: X86_64_RELOC_BRANCH
> > + length: 2
> > + pc-rel: true
> > + extern: true
> > + symbol: 0
> > + - segment: __TEXT
> > + section: __gcc_except_tab
> > + type: S_REGULAR
> > + attributes: [ ]
> > + address: 0x000000000000001c
> > + content: [ 0x00, 0x00, 0x00, 0x00 ]
> > + - segment: __TEXT
> > + section: __eh_frame
> > + type: S_COALESCED
> > + attributes: [ ]
> > + address: 0x0000000000000020
> > + content: [ 0x1c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > + 0x01, 0x7a, 0x50, 0x4c, 0x52, 0x00, 0x01, 0x78,
> > + 0x10, 0x07, 0x9b, 0x04, 0x00, 0x00, 0x00, 0x10,
> > + 0x10, 0x0c, 0x07, 0x08, 0x90, 0x01, 0x00, 0x00,
> > + 0x2c, 0x00, 0x00, 0x00, 0x24, 0x00, 0x00, 0x00,
> > + 0xB8, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > + 0x19, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > + 0x08, 0xc3, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
> > + 0xff, 0x41, 0x0e, 0x10, 0x86, 0x02, 0x43, 0x0d,
> > + 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 ]
> > + - segment: __DATA
> > + section: __data
> > + type: S_REGULAR
> > + attributes: [ ]
> > + address: 0x0000000000000068
> > + content: [ 0x00, 0x00, 0x00, 0x00 ]
> > +local-symbols:
> > + - name: _catchMyException
> > + type: N_SECT
> > + sect: 1
> > + value: 0x0000000000000000
> > +undefined-symbols:
> > + - name: _foo
> > + type: N_UNDF
> > + scope: [ N_EXT ]
> > + value: 0x0000000000000000
> > + - name: ___cxa_begin_catch
> > + type: N_UNDF
> > + scope: [ N_EXT ]
> > + value: 0x0000000000000000
> > + - name: ___cxa_end_catch
> > + type: N_UNDF
> > + scope: [ N_EXT ]
> > + value: 0x0000000000000000
> > +...
> > +
> > +# CHECK: - type: unwind-cfi
> > +# CHECK-NOT: - type:
> > +# CHECK: references:
> > +# CHECK-NEXT: - kind: negDelta32
> > +# CHECK-NEXT: offset: 4
> > +# CHECK-NEXT: target: L000
> > +# CHECK-NEXT: - kind: unwindFDEToFunction
> > +# CHECK-NEXT: offset: 8
> > +# CHECK-NEXT: target: _catchMyException
> > +# CHECK-NEXT: - kind: unwindFDEToFunction
> > +# CHECK-NEXT: offset: 25
> > +# CHECK-NEXT: target: L001
>
>
> On Tue, May 12, 2015 at 12:44 PM, Lang Hames <lhames at gmail.com> wrote:
>
>> Hi All,
>>
>> The LSDA field of MachO eh-frames wasn't being properly relocated, and
>> this was causing failures on some EH tests in the nightly test suite. This
>> patch properly relocates the LSDA field, fixing those tests.
>>
>> Cheers,
>> Lang.
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150512/0055df06/attachment.html>
More information about the llvm-commits
mailing list