This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Prevent data inside text sections in assembly
ClosedPublic

Authored by aardappel on Feb 1 2021, 5:22 PM.

Details

Summary

This is not supported in Wasm, unless the data was encoded instructions, but that wouldn't work with the assembler's other functionality (enforcing nesting etc.).

Fixes: https://bugs.llvm.org/show_bug.cgi?id=48971

Diff Detail

Event Timeline

aardappel created this revision.Feb 1 2021, 5:22 PM
aardappel requested review of this revision.Feb 1 2021, 5:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2021, 5:22 PM
aardappel updated this revision to Diff 320643.Feb 1 2021, 5:30 PM
aardappel added inline comments.Feb 1 2021, 5:36 PM
llvm/lib/MC/WasmObjectWriter.cpp
494

Note this check may strictly not be necessary anymore, but the unchecked dereference that was there before seemed a bit fragile for an invariant established elsewhere. Maybe better as an assert?

sbc100 accepted this revision.Feb 1 2021, 5:58 PM

lgtm with a test for the parse error.

llvm/lib/MC/MCParser/WasmAsmParser.cpp
224 ↗(On Diff #320643)

CurrentSection?

235 ↗(On Diff #320643)

I think it would be good to have a test for this user-facing error.. partly because I think we could use more .s file tests in general and the tests kind of act as documentation for what is and isn't possible with our .s format.

This revision is now accepted and ready to land.Feb 1 2021, 5:58 PM
aardappel updated this revision to Diff 321571.Feb 4 2021, 2:46 PM
  • Moved checking of type to AsmParser label parsing.
  • Improved MCSymbolWasm default type handling slightly.
  • Added error test case.
sbc100 accepted this revision.Feb 4 2021, 4:18 PM
sbc100 added inline comments.
llvm/include/llvm/MC/MCSymbolWasm.h
52

Any reason to keep the deprecated method around? Why not just remove it? I guess a followup that removes it would be fine too.

llvm/test/MC/WebAssembly/basic-assembly-errors.s
7

wasm or Wasm rather than WASM.

aardappel updated this revision to Diff 321869.Feb 5 2021, 1:45 PM
aardappel marked an inline comment as done.

Further refactor the symbol type accessors

aardappel added inline comments.Feb 5 2021, 1:46 PM
llvm/include/llvm/MC/MCSymbolWasm.h
52

Was able to get rid of the deprecated method.

sbc100 added inline comments.Feb 5 2021, 1:52 PM
llvm/lib/MC/WasmObjectWriter.cpp
1706

So we still can end up with symbol that don't have exploit type and in that case we default to data symbols?