[WebAssembly] Pass ownership of body to SyntheticFunction . NFC

[ncw]

Summary

This avoids the Writer unnecessarily having a member to retain ownership of the function body.

[ncw]

wasm/Writer.cpp

866 ↗

(On Diff #136484)

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

[sbc100]

wasm/InputChunks.h

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

wasm/Writer.cpp

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]

wasm/Writer.cpp

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 with review comments. Thanks!

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

[ruiu]

LGTM

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.

wasm/InputChunks.h

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.

lld/trunk/wasm/InputChunks.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_WASM_INPUT_CHUNKS_H
 #define LLD_WASM_INPUT_CHUNKS_H
 
 #include "Config.h"
 #include "InputFiles.h"
 #include "lld/Common/ErrorHandler.h"
+#include "lld/Common/Strings.h"
 #include "llvm/Object/Wasm.h"
 
 using llvm::object::WasmSegment;
 using llvm::wasm::WasmFunction;
 using llvm::wasm::WasmRelocation;
 using llvm::wasm::WasmSignature;
 using llvm::object::WasmSection;
 
protected:
 
   const WasmFunction *Function;
   llvm::Optional<uint32_t> OutputIndex;
   llvm::Optional<uint32_t> TableIndex;
 };
 
 class SyntheticFunction : public InputFunction {
 public:
-  SyntheticFunction(const WasmSignature &S, ArrayRef<uint8_t> Body,
-                    StringRef Name)
-      : InputFunction(S, nullptr, nullptr), Name(Name), Body(Body) {}
+  SyntheticFunction(const WasmSignature &S, std::string Body, StringRef Name)
+      : InputFunction(S, nullptr, nullptr), Name(Name), Body(std::move(Body)) {}
 
   StringRef getName() const override { return Name; }
 
 protected:
-  ArrayRef<uint8_t> data() const override { return Body; }
+  ArrayRef<uint8_t> data() const override { return toArrayRef(Body); }
 
   StringRef Name;
-  ArrayRef<uint8_t> Body;
+  std::string Body;
 };
 
 } // namespace wasm
 
 std::string toString(const wasm::InputChunk *);
 } // namespace lld
 
 #endif // LLD_WASM_INPUT_CHUNKS_H

lld/trunk/wasm/Writer.cpp

 #include "InputChunks.h"
 #include "InputGlobal.h"
 #include "OutputSections.h"
 #include "OutputSegment.h"
 #include "SymbolTable.h"
 #include "WriterUtils.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
-#include "lld/Common/Strings.h"
 #include "lld/Common/Threads.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/BinaryFormat/Wasm.h"
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/LEB128.h"
 
private:
   std::vector<const Symbol *> SymtabEntries;
   std::vector<WasmInitEntry> InitFunctions;
 
   // Elements that are used to construct the final output
   std::string Header;
   std::vector<OutputSection *> OutputSections;
 
   std::unique_ptr<FileOutputBuffer> Buffer;
-  std::string CtorFunctionBody;
 
   std::vector<OutputSegment *> Segments;
   llvm::SmallDenseMap<StringRef, OutputSegment *> SegmentMap;
 };
 
 } // anonymous namespace
 
 static void debugPrint(const char *fmt, ...) {

 static const int OPCODE_END = 0xb;
 
 // Create synthetic "__wasm_call_ctors" function based on ctor functions
 // in input object.
 void Writer::createCtorFunction() {
   uint32_t FunctionIndex = NumImportedFunctions + InputFunctions.size();
   WasmSym::CallCtors->setOutputIndex(FunctionIndex);
 
-  // First write the body bytes to a string.
-  std::string FunctionBody;
+  // First write the body's contents to a string.
+  std::string BodyContent;
   {
-    raw_string_ostream OS(FunctionBody);
+    raw_string_ostream OS(BodyContent);
     writeUleb128(OS, 0, "num locals");
     for (const WasmInitEntry &F : InitFunctions) {
       writeU8(OS, OPCODE_CALL, "CALL");
       writeUleb128(OS, F.Sym->getOutputIndex(), "function index");
     }
     writeU8(OS, OPCODE_END, "END");
   }
 
   // Once we know the size of the body we can create the final function body
-  raw_string_ostream OS(CtorFunctionBody);
-  writeUleb128(OS, FunctionBody.size(), "function size");
-  OS.flush();
-  CtorFunctionBody += FunctionBody;
+  std::string FunctionBody;
+  {
+    raw_string_ostream OS(FunctionBody);
+    writeUleb128(OS, BodyContent.size(), "function size");
+    OS << BodyContent;
+  }
 
   const WasmSignature *Sig = WasmSym::CallCtors->getFunctionType();
   SyntheticFunction *F = make<SyntheticFunction>(
-      *Sig, toArrayRef(CtorFunctionBody), WasmSym::CallCtors->getName());
+      *Sig, std::move(FunctionBody), WasmSym::CallCtors->getName());
 
   F->setOutputIndex(FunctionIndex);
   F->Live = true;
   WasmSym::CallCtors->Function = F;
   InputFunctions.emplace_back(F);
 }
 
 // Populate InitFunctions vector with init functions from all input objects.