This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly][yaml2obj][obj2yaml] Elem sections for nonzero tables
ClosedPublic

Authored by wingo on Mar 4 2021, 1:48 AM.

Details

Summary

With reference types, tables can have non-zero table numbers. This
commit adds support for element sections against these tables.

Diff Detail

Event Timeline

wingo created this revision.Mar 4 2021, 1:48 AM
wingo requested review of this revision.Mar 4 2021, 1:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 1:48 AM
wingo added inline comments.Mar 4 2021, 1:54 AM
llvm/lib/MC/WasmObjectWriter.cpp
937

inline comments from @sbc100, from https://reviews.llvm.org/D92321?id=323966#inline-917851:

Paging this back in -- this code was not dead. See the definition for WASM_ELEM_SEGMENT_HAS_ELEM_KIND. I have restored it here. WDYT?

Oh I see.. WASM_ELEM_SEGMENT_HAS_ELEM_KIND is defined as ~SOME_OTHER_FLAG.. that seem odd at best. Perhaps better write to little helper function elemSegmentNeedsElemKind() rather than trying to make that part of the enum? Otherwise folks will think that WASM_ELEM_SEGMENT_HAS_ELEM_KIND is just another flag they can | together. Either that or maybe add MASK to its name and take it out of the enum with the others.

It looks like WASM_ELEM_SEGMENT_HAS_ELEM_KIND is basically PASSIVE | HAS_TABLE_NUMBER ... maybe its more clear to just write that out here?

wingo updated this revision to Diff 328080.Mar 4 2021, 2:03 AM

Add _MASK to HAS_ELEM_KIND name

wingo added inline comments.Mar 4 2021, 2:07 AM
llvm/lib/MC/WasmObjectWriter.cpp
937

Given that we need the logic in 4 different files, it seemed like defining the mask in a central place would be a good idea. You're right though that as it was, you could get confused and think that (WASM_ELEM_SEGMENT_HAS_TABLE_NUMBER & WASM_ELEMENT_SEGMENT_HAS_ELEM_KIND) == 0. I changed the name therefore to WASM_ELEM_SEGMENT_MASK_HAS_ELEM_KIND; wdyt?

There's no accounting for taste, obviously, and if you have a strong preference here, I am very happy to adapt.

I think all the other inline comments from D92321 have been addressed in this patch, fwiw.

sbc100 accepted this revision.Mar 4 2021, 8:03 AM

lgtm!

llvm/include/llvm/BinaryFormat/Wasm.h
316

Do you think this should outside the enum perhaps?

llvm/lib/MC/WasmObjectWriter.cpp
919

It looks like we pass IndirectFunctionTable just to get the table nunber.

Would it make sense to instead change the signature of this functions to:

void WasmObjectWriter::writeElemSection(uint32_t TableNumber, ArrayRef<uint32_t> TableElems)

And then it can be re-used later once we support different tables? Or maybe that be part of the followup?

lgtm which ever way you think makes sense.

This revision is now accepted and ready to land.Mar 4 2021, 8:03 AM
wingo added inline comments.Mar 5 2021, 2:38 AM
llvm/lib/MC/WasmObjectWriter.cpp
919

Yeah that's right. Would work fine to pass a table number, only except that if TableElems is empty, it could be that there's no function table symbol. Once we start having different / other tables, we may want to split things; there may be multiple segments against different tables, for example. If OK with you, will reserve any needed refactors for a followup.

wingo added inline comments.Mar 5 2021, 2:48 AM
llvm/include/llvm/BinaryFormat/Wasm.h
316

fixed when pushed as

const unsigned WASM_ELEM_SEGMENT_MASK_HAS_ELEM_KIND = 0x3;