This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Fix list of relocations with addends in lld
ClosedPublic

Authored by loladiro on Jun 23 2019, 4:24 PM.

Details

Summary

The list of relocations with addend in lld was missing R_WASM_MEMORY_ADDR_REL_SLEB,
causing wasm-ld to generate corrupted output. This fixes that problem and while
we're at it pulls the list of such relocations into the Wasm.h header, to avoid
duplicating it in multiple places.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro created this revision.Jun 23 2019, 4:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2019, 4:24 PM

Thanks!

I guess this means we are also missing a test case for --emit-relocs + -fPIC?

llvm/include/llvm/BinaryFormat/Wasm.h
169 ↗(On Diff #206147)

Rather than adding methods to this struct can you implement in the same way that relocTypetoString is below?

Perhaps we can make them methods later, but I'm currently a fan of keeping the struct definitions clean so they can be easily matched with the binary spec.

sbc100 requested changes to this revision.Jun 25 2019, 4:09 PM
This revision now requires changes to proceed.Jun 25 2019, 4:09 PM
loladiro updated this revision to Diff 206550.Jun 25 2019, 5:03 PM

Address review comments and add test case.

sbc100 accepted this revision.Jun 25 2019, 5:26 PM

lgtm with comment

lld/test/wasm/emit-relocs-fpic.ll
1 ↗(On Diff #206550)

Can you name this file .s since it contains asm?

(I think this is the first of the .s files tests in lld so you will also need to add .s to the list of suffixes in lld/test/wasm/lit.local.cfg)

This revision is now accepted and ready to land.Jun 25 2019, 5:26 PM
loladiro marked an inline comment as done.Jun 25 2019, 5:27 PM
loladiro added inline comments.
lld/test/wasm/emit-relocs-fpic.ll
1 ↗(On Diff #206550)

Good catch. Meant to do that, but forgot. Will do and commit.

This revision was automatically updated to reflect the committed changes.

There was a small omission in this revision that I fixed up in https://reviews.llvm.org/rG5bb0dcd96ec1.