Page MenuHomePhabricator

[WebAssembly] Pass ownership of body to SyntheticFunction . NFC

Authored by ncw on Mar 1 2018, 2:27 AM.

Diff Detail


Event Timeline

ncw created this revision.Mar 1 2018, 2:27 AM
ncw added inline comments.Mar 1 2018, 8:19 AM
866 ↗(On Diff #136484)

Oops, my IDE "fixed" this for me :( I'll correct this nit before landing.

sbc100 added inline comments.Mar 1 2018, 8:43 AM
156 ↗(On Diff #136484)

Seems reasonable. I'm not sure if the ownership passing and move semantics stuff is overkill here in lld land though. Seems like make<> and the arenaAllocator might be preferred/simpler. I'll defer to @ruiu

870 ↗(On Diff #136484)

The block scope here was intended to automatically flush and destroy the OS object.

I suggest you move BodyContent outside this block, then create a second block for OS2 (in which you can actually call it OS, since it will be separate scope.

The OS2.flush() below is redundant since the scope will trigger that flush when the stream is destroyed.

Seems strange that FunctionBody and BodyContent are not the same type. Can they be?

899 ↗(On Diff #136484)

How about just assert(*Sym->getFunctionType() == WasmSignature{{}, WASM_TYPE_NORESULT});

Then the release build won't need the extra variable.

This seems like its really a separate change.

ruiu added inline comments.Mar 1 2018, 8:49 AM
893 ↗(On Diff #136484)

Remove this variable and inline it. Otherwise you'll get a "unused variable" warning for non-NDEBUG build.

899 ↗(On Diff #136484)

If you can trigger this by feeding an object file with a bad init function, you should use error() instead of assert(). assert() is for checking internal consistency only.

ncw updated this revision to Diff 136548.Mar 1 2018, 9:08 AM
ncw marked 4 inline comments as done.
ncw retitled this revision from [WebAssembly] Pass ownership of body to SyntheticFunction and assert type. NFC to [WebAssembly] Pass ownership of body to SyntheticFunction . NFC.
ncw edited the summary of this revision. (Show Details)

Updated with review comments. Thanks!

Assertion pulled out into another review, you're right it's separate.

ruiu added a comment.Mar 1 2018, 10:13 AM


It feels like there's some way to simplify the code in other way though. One idea is to add OS member to a synthetic section and add contents to that stream, instead of creating a function body beforehand and pass it to the synthetic function's constructor. Maybe there are other possibilities too. But I don't think that's not very important at the moment.

156 ↗(On Diff #136484)

Yeah, that's good point. I was thinking about that. In lld, we don't do memory management per object since almost all heap-allocated objects are needed until the last moment when we flush everything to disk. That said, I'm perhaps OK with this. This is a small piece of code, and we can change later when we change the mind.

sbc100 accepted this revision.Mar 1 2018, 10:18 AM
This revision is now accepted and ready to land.Mar 1 2018, 10:18 AM
This revision was automatically updated to reflect the committed changes.