This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] MC: Don't allow zero sized data segments
ClosedPublic

Authored by sbc100 on Oct 19 2017, 1:46 PM.

Details

Summary

This ensures that each segment has a unique address.
Without this, consecutive zero sized symbols would
end up with the same address and the linker cannot
map symbols to unique data segments.

Event Timeline

sbc100 created this revision.Oct 19 2017, 1:46 PM
sbc100 added a subscriber: llvm-commits.
dschuff edited edge metadata.Oct 20 2017, 1:20 PM

Should symbols actually be allowed to have the same value? How do we handle aliases?

Symbols can have the same value. The problem I was running into was when a zero sized symbol was at the end of segment its not possible know if its pointing to the end one of segment of the beginning on the next. An alternative would be store the segment index for each symbol.

dschuff added inline comments.Oct 20 2017, 5:27 PM
lib/MC/WasmObjectWriter.cpp
526

should we set LastFragmentSize to the fill size here and/or for align frags above? I guess if the answer is "no" it's because they do not represent symbols?

sbc100 added inline comments.Oct 23 2017, 3:44 PM
lib/MC/WasmObjectWriter.cpp
526

Basically yes. Zero sized fill fragments do seem to exist but don't represent symbols. They seems to be generated to inject padded between elements of global arrays. So I don't think its possible to have a symbol point to them.

I do think we might need a better solution to this issue in the long run though. Perhaps adding specifying explicitly which segment a symbol belongs too?

dschuff accepted this revision.Oct 23 2017, 4:20 PM

Yeah, this seems OK for now.
I think at some point we may want to revisit the overall goals of the representation that we discussed originally (i.e. not having to encode a symbol table outside of wasm globals, a wasm object file possibly being directly loadable, etc), once we have a prototype and the constraints are clearer. Probably then it will be more clear what to do in cases like this.

This revision is now accepted and ready to land.Oct 23 2017, 4:20 PM
This revision was automatically updated to reflect the committed changes.