This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly][MC] Fix placement of table section
ClosedPublic

Authored by wingo on Nov 30 2020, 7:18 AM.

Details

Summary

The table section goes after functions.

Diff Detail

Event Timeline

wingo created this revision.Nov 30 2020, 7:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2020, 7:18 AM
wingo requested review of this revision.Nov 30 2020, 7:18 AM
sbc100 added inline comments.Nov 30 2020, 7:23 AM
llvm/lib/MC/WasmObjectWriter.cpp
1790

I don't understand how moving the table section before Events and Globals relates to being able to use externref and funcref as block types.

Aren't all types defined in the type section? And also.. the blocks themselves are all defined in Code section which is after all of these isn't it?

wingo added inline comments.Dec 1 2020, 12:21 AM
llvm/lib/MC/WasmObjectWriter.cpp
1790

They are only related in the sense that they were just two small things that I needed to fix to assemble and link https://github.com/Igalia/ref-cpp/blob/master/milestones/m3/test.S.

Regarding this specific block, the table section must go after functions and before events. The existing tests only validated before because the tests didn't include events or globals. Once you start linking things with globals, you run into validation problems, both in wasm-ld when it parses object files, and in external tools.

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
343 ↗(On Diff #308364)

I assume this will be subsumed by https://reviews.llvm.org/D92359

sbc100 added inline comments.Dec 1 2020, 1:06 AM
llvm/lib/MC/WasmObjectWriter.cpp
1790

Wait.. are you saying that any llvm-produced object file today that contains even one global or event will not validate?

wingo added inline comments.Dec 1 2020, 1:12 AM
llvm/lib/MC/WasmObjectWriter.cpp
1790

Any LLVM-produced object file which defines a table and which contains even one global will not validate, correct.

We don't hit this case right now because LLVM doesn't produce object files that define tables, unless in a .s file -- there is only __indirect_function_table which is an import, not a definition.

sbc100 accepted this revision.Dec 1 2020, 1:13 AM

Oh I see .. up until now we import the table.

This revision is now accepted and ready to land.Dec 1 2020, 1:13 AM
sbc100 added a comment.Dec 1 2020, 1:14 AM

I guess we wait for D92359 and then land this on top of that.

Have you applied for commit access yet?

wingo added a comment.Dec 1 2020, 1:23 AM

I guess we wait for D92359 and then land this on top of that.

SGTM

Have you applied for commit access yet?

Not yet, will do that now. Thanks for the poke.

This revision was landed with ongoing or failed builds.Dec 7 2020, 7:19 AM
This revision was automatically updated to reflect the committed changes.