This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add support for user-defined custom sections
ClosedPublic

Authored by sbc100 on Apr 5 2018, 3:41 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Apr 5 2018, 3:41 PM
sbc100 updated this revision to Diff 141234.Apr 5 2018, 3:45 PM
  • cleanup
sbc100 retitled this revision from [WebAssembly] Add support for custom sections to [WebAssembly] Add support for user-defined custom sections.
ruiu added a comment.Apr 5 2018, 4:01 PM

I think overall it is looking good.

wasm/InputChunks.h
185 ↗(On Diff #141234)

If you move this to .cpp, can you remove #include "LEB128.h"?

Please add a comment what this code is doing.

196 ↗(On Diff #141234)

It may make sense to do this in the ctor and store an ArrayRef instead of an size_t offset as a member.

198–200 ↗(On Diff #141234)

What is this function? Please add a comment to InputChunk class.

wasm/OutputSections.h
116 ↗(On Diff #141234)

Please add a class comment.

118 ↗(On Diff #141234)

I wouldn't add explicit because it is hard to accidentally trigger implicit type conversion for this class.

wasm/Writer.cpp
314 ↗(On Diff #141234)

Are you sure that these section names don't start with "."?

330 ↗(On Diff #141234)

You can eliminate this variable.

sbc100 updated this revision to Diff 141238.Apr 5 2018, 4:35 PM
sbc100 marked 6 inline comments as done.
  • feedback
sbc100 added inline comments.Apr 5 2018, 4:35 PM
wasm/Writer.cpp
314 ↗(On Diff #141234)

Yes. These sections are all created explicitly with these names

yurydelendik accepted this revision.Apr 9 2018, 1:51 PM

Looks good to me (from D45118 point of view)

This revision is now accepted and ready to land.Apr 9 2018, 1:51 PM
sbc100 added a comment.Apr 9 2018, 3:58 PM

@ruiu , good to go now?

ruiu added inline comments.Apr 9 2018, 4:05 PM
wasm/OutputSections.h
116 ↗(On Diff #141234)

Can you expand the comment so that those who don't really now what "custom section" is can understand this comment? Currently, in order to understand this comment, you need to know what custom section is before reading this comment.

You need to explain that in wasm, text, data, and some other sections are special sections that are identified by type, and unlike other file formats, they don't have name. However, you can still add sections that don't have special type and have name just like normal sections in other file formats, and such sections are called "custom sections" in wasm.

wasm/Writer.cpp
304–305 ↗(On Diff #141238)

nit: we omit { if the content is only one line.

sbc100 edited the summary of this revision. (Show Details)Apr 9 2018, 5:04 PM
sbc100 updated this revision to Diff 141769.Apr 9 2018, 5:17 PM
sbc100 marked 2 inline comments as done.
  • feedback
ruiu accepted this revision.Apr 9 2018, 5:24 PM

LGTM

This revision was automatically updated to reflect the committed changes.