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