[PATCH] D96659: [WebAssembly] Add new relocation for location relative data

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Mostly lgtm.  Just a few nits at this point.

I don't love the name `SELFREL` but naming is hard and I'm not sure I can think of a better one. (LOCREL for location-relative?)    Also, since can renaming the enum later without breaking compat we are not locked into the name.   I wonder if we should rename of the existing `REL` relocation types to `MBREL` and `TBREL` to be explicit there too?  (Not suggesting you do that as part of this change).



================
Comment at: lld/test/wasm/selfrel-reloc.ll:27
+    i32 ptrtoint (i32* @fizz to i32)
+), section ".sec2"
+
----------------
Can we write this test in assembly?   I've been trying to convert all the lld test to asm so would rather not add more `.ll` tests here.   Also I think it might be a lore more concise/readable perhaps in .s form?


================
Comment at: lld/wasm/InputFiles.cpp:183
+        wasmObj->dataSegments()[sym.Info.DataRef.Segment];
+    return segment.Data.Offset.Value.Int32;
+  }
----------------
Is this correct?   It looks like we are expecting the value written to the location to be the start of the segment containing the symbol?  (Maybe this works because we default to `-fdata-sections`?)


================
Comment at: llvm/test/MC/WebAssembly/selfrel.ll:1
+; RUN: llc -O0 -filetype=obj %s -o - | llvm-readobj -r --expand-relocs - | FileCheck %s
+
----------------
Can we give this test file a more descriptive name?   How about `reloc-relative.ll` or `reloc-selfrel.ll` ?   (to match the other tests in this directory that start with `reloc-`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96659/new/

https://reviews.llvm.org/D96659