Page MenuHomePhabricator

[WebAssembly] MC: Fix for data aliases with offsets (getelementptr)
ClosedPublic

Authored by sbc100 on May 5 2020, 6:08 PM.

Details

Summary

For some reason we hadn't seen such cases in the wild which makes
me think that clang and rustc don't generate these. In the bug which
reproduces it only occurs with LTO so my guess is that some LTO pass
is creating this alias + gep.

See: https://github.com/emscripten-core/emscripten/issues/8731

Of the 3 new tests added here the first 2 already passed, and
test/MC/WebAssembly/offset.s would previously crash.

Diff Detail

Event Timeline

sbc100 created this revision.May 5 2020, 6:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2020, 6:08 PM
sbc100 edited the summary of this revision. (Show Details)May 5 2020, 6:12 PM
sbc100 added reviewers: dschuff, kripken.
sbc100 edited reviewers, added: aheejin; removed: kripken.May 6 2020, 9:55 AM
aheejin added inline comments.May 7 2020, 9:13 AM
llvm/test/CodeGen/WebAssembly/aliases.ll
3

Can we also check if llvm-mc can correctly parse the result of this test? That will possibly break the current codebase.

llvm/test/MC/WebAssembly/offset.s
13

Does this handle something like

.set sym_c, 16

too? The code assumes there always exists a base symbol.

sbc100 marked 2 inline comments as done.May 7 2020, 10:00 AM
sbc100 added inline comments.
llvm/test/CodeGen/WebAssembly/aliases.ll
3

None of the other tests in this directory to do that. In general I see this directory as testing CodeGen only. The ability for MC to parse this stuff is tested separately in llvm/test/MC/WebAssembly/. See the two new tests I added there.

llvm/test/MC/WebAssembly/offset.s
13

Yes, you are right it looks like we don't support absolute addresses like this yet. If its OK with you I'll leave that as a followup change? We might need a different symbol flag for that because currently all our data symbols need to be relative to a segment,.

aheejin accepted this revision.May 7 2020, 10:22 AM
aheejin added inline comments.
llvm/test/MC/WebAssembly/offset.s
13

Yeah sounds good.

This revision is now accepted and ready to land.May 7 2020, 10:22 AM
sbc100 updated this revision to Diff 271517.Jun 17 2020, 4:25 PM
  • rebase
This revision was automatically updated to reflect the committed changes.