This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Ensure relocation entries are ordered by offset
ClosedPublic

Authored by sbc100 on Aug 21 2018, 2:21 PM.

Diff Detail

Event Timeline

sbc100 created this revision.Aug 21 2018, 2:21 PM
sbc100 retitled this revision from [WebAssembly] Ensure relocation are entries are ordered by offset to [WebAssembly] Ensure relocation entries are ordered by offset.Aug 21 2018, 2:24 PM
sbc100 edited the summary of this revision. (Show Details)Aug 21 2018, 4:25 PM
ncw accepted this revision.Aug 22 2018, 1:43 AM
ncw added a subscriber: ncw.

Looks good to me. I think something similar to this was included in one of my previous patches, and just got lost somewhere.

I'm busy on other things at work, so I've had to put Wasm to the side for the last few months, but I'm hoping to get back to polishing and submitting some more stuff - when I'm able!

lib/MC/WasmObjectWriter.cpp
918–922

Should this be stable_sort? (I think LLVM is trying to remove uses of the unstable/non-deterministic version.)

Alternatively could assert that it's sorted, since it seems in practice that it is (and the assert would make sure that stays the case).

This revision is now accepted and ready to land.Aug 22 2018, 1:43 AM
sbc100 updated this revision to Diff 161986.Aug 22 2018, 10:14 AM
  • fixup
  • stable_sort
lib/MC/WasmObjectWriter.cpp
918–922

I tracked down some cases where its not ordered.

The issue is that we order the function according to the order of the Asm.symbols() list, an not the order of the sections in Asm. If we followed the sections order (I wrote a patch for this to confirm) we would most likely be ordered in most cases today, but I'm not sure that is guaranteed anywhere in LLVM. So I think certainly now we need to sort. Other plaforms do this reloc sorting too. See MCELFObjectTargetWriter::sortRelocs

This revision was automatically updated to reflect the committed changes.