Page MenuHomePhabricator

[WebAssembly][MC] Fix computation of relative symbol offset
ClosedPublic

Authored by ddcc on Sep 9 2020, 12:06 PM.

Details

Summary

For relative symbols, add its offset when computing relocation value.

Diff Detail

Event Timeline

ddcc created this revision.Sep 9 2020, 12:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 12:06 PM
ddcc requested review of this revision.Sep 9 2020, 12:06 PM
ddcc added inline comments.Sep 9 2020, 12:07 PM
llvm/test/MC/WebAssembly/alias-relative.ll
31 ↗(On Diff #290790)

I haven't been able to get obj2yaml or llvm-readobj to print out the full relocation information. Is there a more appropriate tool to use?

Thanks for fixing this! Sorry it took me so long to get back to you. This lgtm % some comments.

Is this essentially a fix for the work that was started in https://reviews.llvm.org/D7946 ?

If we I wonder if the existing test for aliases at offset could be extended to include a relocation: llvm/test/MC/WebAssembly/offset.s? Rather than adding a new test?

I'd be happy to rename the existing test to alias-relative.ll or alias-offset.ll which seem more meaningful.

We are currently trying to move away from .ll tests in the MC directory to .s where possible so even if we do need to add a new test .s would probably be prefered.

llvm/lib/MC/WasmObjectWriter.cpp
1233

Is this a separate issue? It looks like a possible crash. Did you hit this case during testing? I'm curious what conditions would cause this.

llvm/test/MC/WebAssembly/alias-relative.ll
31 ↗(On Diff #290790)

The relocation is shown above.. what more information are you looking for?

I would recommend objdump -d -r but I'm not sure its works well enough with WebAssembly at this point.

ddcc updated this revision to Diff 293327.Sep 21 2020, 8:47 PM
ddcc marked an inline comment as done.

Merge tests

ddcc added a comment.Sep 21 2020, 8:47 PM

Is this essentially a fix for the work that was started in https://reviews.llvm.org/D79462 ?

Yeah, and enabling CFI will enable the LowerTypeTests pass which can generate relative addressing.

If we I wonder if the existing test for aliases at offset could be extended to include a relocation: llvm/test/MC/WebAssembly/offset.s? Rather than adding a new test?
I'd be happy to rename the existing test to alias-relative.ll or alias-offset.ll which seem more meaningful.
We are currently trying to move away from .ll tests in the MC directory to .s where possible so even if we do need to add a new test .s would probably be prefered.

Ok, this is pretty small so I can just merge it into the existing test.

llvm/lib/MC/WasmObjectWriter.cpp
1233

Currently, if you enable CFI for WebAssembly (-fsanitize=cfi -fvisibility=hidden), the LowerTypeTests pass will generate a null base pointer as an absolute address, which eventually triggers here. It was a workaround from back when I originally implemented CFI support; D87258 reworks the WAsm portion of that pass to avoid it.

llvm/test/MC/WebAssembly/alias-relative.ll
31 ↗(On Diff #290790)

Never mind, I misunderstood the relocation behavior.

sbc100 accepted this revision.Sep 21 2020, 9:39 PM

Great!

llvm/test/MC/WebAssembly/alias-offset.s
2

If you add -r here will it also show the relocation information in the objdump output? (I can't remember if that works with wasm or not).

This revision is now accepted and ready to land.Sep 21 2020, 9:39 PM
ddcc updated this revision to Diff 293331.Sep 21 2020, 9:42 PM

Check relocation output

ddcc marked an inline comment as done.Sep 21 2020, 9:42 PM
ddcc added inline comments.
llvm/test/MC/WebAssembly/alias-offset.s
2

Yup, added.

Do you have commit access or should I land this for you?

This revision was landed with ongoing or failed builds.Sep 21 2020, 9:53 PM
This revision was automatically updated to reflect the committed changes.
ddcc added a comment.Sep 21 2020, 9:55 PM

Landed, mind looking at https://reviews.llvm.org/D87258#2267267 when you get the time?