From b49a798e71f922a68628ad9e31ca12fdb864c2f5 Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Tue, 11 May 2021 11:08:14 -0700 Subject: [PATCH] [lld][WebAssembly] Remove relocation target verification We have this extra step in wasm-ld that doesn't exist in other lld backend which verifies the existing contents of the relocation targets. This was originally intended as an extra form of double checking and an aid to compiler developers. However it has always been somewhat controversial and there have been suggestions in the past the we simply remove it. My motivation for removing it now is that its causing me a headache when trying to fix an issue with negative addends. In the case of negative addends that final result can be wrapped/negative but this checking code would require significant modification to be able to deal with that case. For example with some test cases I'm looking at I'm seeing error like this: ``` wasm-ld: warning: /usr/local/google/home/sbc/dev/wasm/llvm-build/tools/lld/test/wasm/Output/merge-string.s.tmp.o:(.rodata_relocs): unexpected existing value for R_WASM_MEMORY_ADDR_I32: existing=FFFFFFFA expected=FFFFFFFFFFFFFFFA ``` Rather than try to refactor `calcExpectedValue` to somehow return two different types of results (32 and 64-bit) depending on the relocation type, I think we can just remove this code. Differential Revision: https://reviews.llvm.org/D102265 --- lld/test/wasm/reloc-addend.s | 11 +++++++ lld/wasm/InputChunks.cpp | 68 -------------------------------------------- lld/wasm/InputFiles.cpp | 65 ------------------------------------------ lld/wasm/InputFiles.h | 1 - 4 files changed, 11 insertions(+), 134 deletions(-) diff --git a/lld/test/wasm/reloc-addend.s b/lld/test/wasm/reloc-addend.s index 56c3296..acd57ce 100644 --- a/lld/test/wasm/reloc-addend.s +++ b/lld/test/wasm/reloc-addend.s @@ -23,9 +23,20 @@ bar: .int32 foo+64 .size bar, 4 +# Check that negative addends also work here + .section .data.negative_addend,"",@ + .p2align 2 +negative_addend: + .int32 foo-16 + .size negative_addend, 4 + # CHECK: - Type: DATA # CHECK-NEXT: Relocations: # CHECK-NEXT: - Type: R_WASM_MEMORY_ADDR_I32 # CHECK-NEXT: Index: 0 # CHECK-NEXT: Offset: 0x6 # CHECK-NEXT: Addend: 64 +# CHECK-NEXT: - Type: R_WASM_MEMORY_ADDR_I32 +# CHECK-NEXT: Index: 0 +# CHECK-NEXT: Offset: 0xF +# CHECK-NEXT: Addend: -16 diff --git a/lld/wasm/InputChunks.cpp b/lld/wasm/InputChunks.cpp index 14c0190..f73e336 100644 --- a/lld/wasm/InputChunks.cpp +++ b/lld/wasm/InputChunks.cpp @@ -57,70 +57,6 @@ StringRef InputChunk::getComdatName() const { return file->getWasmObj()->linkingData().Comdats[index]; } -void InputChunk::verifyRelocTargets() const { - for (const WasmRelocation &rel : relocations) { - uint64_t existingValue; - unsigned bytesRead = 0; - unsigned paddedLEBWidth = 5; - auto offset = rel.Offset - getInputSectionOffset(); - const uint8_t *loc = data().data() + offset; - switch (rel.Type) { - case R_WASM_TYPE_INDEX_LEB: - case R_WASM_FUNCTION_INDEX_LEB: - case R_WASM_GLOBAL_INDEX_LEB: - case R_WASM_EVENT_INDEX_LEB: - case R_WASM_MEMORY_ADDR_LEB: - case R_WASM_TABLE_NUMBER_LEB: - existingValue = decodeULEB128(loc, &bytesRead); - break; - case R_WASM_MEMORY_ADDR_LEB64: - existingValue = decodeULEB128(loc, &bytesRead); - paddedLEBWidth = 10; - break; - case R_WASM_TABLE_INDEX_SLEB: - case R_WASM_TABLE_INDEX_REL_SLEB: - case R_WASM_MEMORY_ADDR_SLEB: - case R_WASM_MEMORY_ADDR_REL_SLEB: - case R_WASM_MEMORY_ADDR_TLS_SLEB: - existingValue = static_cast(decodeSLEB128(loc, &bytesRead)); - break; - case R_WASM_TABLE_INDEX_SLEB64: - case R_WASM_MEMORY_ADDR_SLEB64: - case R_WASM_MEMORY_ADDR_REL_SLEB64: - existingValue = static_cast(decodeSLEB128(loc, &bytesRead)); - paddedLEBWidth = 10; - break; - case R_WASM_TABLE_INDEX_I32: - case R_WASM_MEMORY_ADDR_I32: - case R_WASM_FUNCTION_OFFSET_I32: - case R_WASM_SECTION_OFFSET_I32: - case R_WASM_GLOBAL_INDEX_I32: - case R_WASM_MEMORY_ADDR_LOCREL_I32: - existingValue = read32le(loc); - break; - case R_WASM_TABLE_INDEX_I64: - case R_WASM_MEMORY_ADDR_I64: - case R_WASM_FUNCTION_OFFSET_I64: - existingValue = read64le(loc); - break; - default: - llvm_unreachable("unknown relocation type"); - } - - if (bytesRead && bytesRead != paddedLEBWidth) - warn("expected LEB at relocation site be 5/10-byte padded"); - - if (rel.Type != R_WASM_GLOBAL_INDEX_LEB && - rel.Type != R_WASM_GLOBAL_INDEX_I32) { - auto expectedValue = file->calcExpectedValue(rel); - if (expectedValue != existingValue) - warn(toString(this) + ": unexpected existing value for " + - relocTypeToString(rel.Type) + ": existing=" + - Twine(existingValue) + " expected=" + Twine(expectedValue)); - } - } -} - // Copy this input chunk to an mmap'ed output file and apply relocations. void InputChunk::writeTo(uint8_t *buf) const { // Copy contents @@ -134,10 +70,6 @@ void InputChunk::relocate(uint8_t *buf) const { if (relocations.empty()) return; -#ifndef NDEBUG - verifyRelocTargets(); -#endif - LLVM_DEBUG(dbgs() << "applying relocations: " << toString(this) << " count=" << relocations.size() << "\n"); int32_t inputSectionOffset = getInputSectionOffset(); diff --git a/lld/wasm/InputFiles.cpp b/lld/wasm/InputFiles.cpp index 99e2d0b..bfb60a3 100644 --- a/lld/wasm/InputFiles.cpp +++ b/lld/wasm/InputFiles.cpp @@ -135,71 +135,6 @@ uint64_t ObjFile::calcNewAddend(const WasmRelocation &reloc) const { } } -// Calculate the value we expect to find at the relocation location. -// This is used as a sanity check before applying a relocation to a given -// location. It is useful for catching bugs in the compiler and linker. -uint64_t ObjFile::calcExpectedValue(const WasmRelocation &reloc) const { - switch (reloc.Type) { - case R_WASM_TABLE_INDEX_I32: - case R_WASM_TABLE_INDEX_I64: - case R_WASM_TABLE_INDEX_SLEB: - case R_WASM_TABLE_INDEX_SLEB64: { - const WasmSymbol &sym = wasmObj->syms()[reloc.Index]; - return tableEntries[sym.Info.ElementIndex]; - } - case R_WASM_TABLE_INDEX_REL_SLEB: { - const WasmSymbol &sym = wasmObj->syms()[reloc.Index]; - return tableEntriesRel[sym.Info.ElementIndex]; - } - case R_WASM_MEMORY_ADDR_LEB: - case R_WASM_MEMORY_ADDR_LEB64: - case R_WASM_MEMORY_ADDR_SLEB: - case R_WASM_MEMORY_ADDR_SLEB64: - case R_WASM_MEMORY_ADDR_REL_SLEB: - case R_WASM_MEMORY_ADDR_REL_SLEB64: - case R_WASM_MEMORY_ADDR_I32: - case R_WASM_MEMORY_ADDR_I64: - case R_WASM_MEMORY_ADDR_TLS_SLEB: - case R_WASM_MEMORY_ADDR_LOCREL_I32: { - const WasmSymbol &sym = wasmObj->syms()[reloc.Index]; - if (sym.isUndefined()) - return 0; - const WasmSegment &segment = - wasmObj->dataSegments()[sym.Info.DataRef.Segment]; - if (segment.Data.Offset.Opcode == WASM_OPCODE_I32_CONST) - return segment.Data.Offset.Value.Int32 + sym.Info.DataRef.Offset + - reloc.Addend; - else if (segment.Data.Offset.Opcode == WASM_OPCODE_I64_CONST) - return segment.Data.Offset.Value.Int64 + sym.Info.DataRef.Offset + - reloc.Addend; - else - llvm_unreachable("unknown init expr opcode"); - } - case R_WASM_FUNCTION_OFFSET_I32: - case R_WASM_FUNCTION_OFFSET_I64: { - const WasmSymbol &sym = wasmObj->syms()[reloc.Index]; - InputFunction *f = - functions[sym.Info.ElementIndex - wasmObj->getNumImportedFunctions()]; - return f->getFunctionInputOffset() + f->getFunctionCodeOffset() + - reloc.Addend; - } - case R_WASM_SECTION_OFFSET_I32: - return reloc.Addend; - case R_WASM_TYPE_INDEX_LEB: - return reloc.Index; - case R_WASM_FUNCTION_INDEX_LEB: - case R_WASM_GLOBAL_INDEX_LEB: - case R_WASM_GLOBAL_INDEX_I32: - case R_WASM_EVENT_INDEX_LEB: - case R_WASM_TABLE_NUMBER_LEB: { - const WasmSymbol &sym = wasmObj->syms()[reloc.Index]; - return sym.Info.ElementIndex; - } - default: - llvm_unreachable("unknown relocation type"); - } -} - // Translate from the relocation's index into the final linked output value. uint64_t ObjFile::calcNewValue(const WasmRelocation &reloc, uint64_t tombstone, const InputChunk *chunk) const { diff --git a/lld/wasm/InputFiles.h b/lld/wasm/InputFiles.h index 78b3cee..dab44cd 100644 --- a/lld/wasm/InputFiles.h +++ b/lld/wasm/InputFiles.h @@ -122,7 +122,6 @@ public: uint64_t calcNewValue(const WasmRelocation &reloc, uint64_t tombstone, const InputChunk *chunk) const; uint64_t calcNewAddend(const WasmRelocation &reloc) const; - uint64_t calcExpectedValue(const WasmRelocation &reloc) const; Symbol *getSymbol(const WasmRelocation &reloc) const { return symbols[reloc.Index]; }; -- 2.7.4