DWARF debug line updating (#2545)

With this, we can update DWARF debug line info properly as
we write a new binary.

To do that we track binary locations as we write. Each
instruction is mapped to the location it is written to. We
must also adjust them as we move code around because
of LEB optimization (we emit a function or a section
with a 5-byte LEB placeholder, the maximal size; later
we shrink it which is almost always possible).

writeDWARFSections() now takes a second param, the new
locations of instructions. It then maps debug line info from the
original offsets in the binary to the new offsets in the binary
being written.

The core logic for updating the debug line section is in
wasm-debug.cpp. It basically tracks state machine logic
both to read the existing debug lines and to emit the new
ones. I couldn't find a way to reuse LLVM code for this, but
reading LLVM's code was very useful here.

A final tricky thing we need to do is to update the DWARF
section's internal size annotation. The LLVM YAML writing
code doesn't do that for us. Luckily it's pretty easy, in
fixEmittedSection we just update the first 4 bytes in place
to have the section size, after we've emitted it and know
the size.

This ignores debug lines with a 0 in the line, col, or addr,
see WebAssembly/debugging#9 (comment)

This ignores debug line offsets into the middle of
instructions, which LLVM sometimes emits for some
reason, see WebAssembly/debugging#9 (comment)
Handling that would likely at least double our memory
usage, which is unfortunate - we are run in an LTO manner,
where the entire app's DWARF is present, and it may be
massive. I think we should see if such odd offsets are
a bug in LLVM, and if we can fix or prevent that.

This does not emit "special" opcodes for debug lines. Those
are purely an optimization, which I wanted to leave for
later. (Even without them we decrease the size quite a lot,
btw, as many lines have 0s in them...)

This adds some testing that shows we can load and save
fib2.c and fannkuch.cpp properly. The latter includes more
than one function and has nontrivial code.

To actually emit correct offsets a few minor fixes are
done here:

*   Fix the code section location tracking during reading -
    the correct offset we care about is the body of the code
    section, not including the section declaration and size.
*   Fix wasm-stack debug line emitting. We need to update
    in BinaryInstWriter::visit(), that is, right before writing
    bytes for the instruction. That differs from
*   BinaryenIRWriter::visit which is a recursive function
    that also calls the children - so the offset there would be
    of the first child. For some reason that is correct with
    source maps, I don't understand why, but it's wrong for
    DWARF...
*   Print code section offsets in hex, to match other tools.
    Remove DWARFUpdate pass, which was useful for testing
    temporarily, but doesn't make sense now (it just updates without
    writing a binary).

cc @yurydelendik

Author: kripken

src/passes/DWARF.cpp

@@ -35,14 +35,6 @@ struct DWARFDump : public Pass {
   }
 };
 
-struct DWARFUpdate : public Pass {
-  void run(PassRunner* runner, Module* module) override {
-    Debug::writeDWARFSections(*module);
-  }
-};
-
 Pass* createDWARFDumpPass() { return new DWARFDump(); }
 
-Pass* createDWARFUpdatePass() { return new DWARFUpdate(); }
-
 } // namespace wasm

src/passes/Print.cpp

@@ -1427,7 +1427,8 @@ struct PrintSExpression : public OverriddenVisitor<PrintSExpression> {
         auto iter = currFunction->binaryLocations.find(curr);
         if (iter != currFunction->binaryLocations.end()) {
           Colors::grey(o);
-          o << ";; code offset: 0x" << iter->second << '\n';
+          o << ";; code offset: 0x" << std::hex << iter->second << std::dec
+            << '\n';
           restoreNormalColor(o);
           doIndent(o, indent);
         }

src/passes/RoundTrip.cpp

@@ -57,7 +57,7 @@ struct RoundTrip : public Pass {
     // Read
     ModuleUtils::clearModule(*module);
     ModuleReader reader;
-    // TODO: enable debug info when relevant
+    reader.setDWARF(runner->options.debugInfo);
     reader.read(tempName, *module);
     // Clean up
     std::remove(tempName.c_str());

src/passes/pass.cpp

@@ -110,8 +110,6 @@ void PassRegistry::registerPasses() {
   registerPass("dwarfdump",
                "dump DWARF debug info sections from the read binary",
                createDWARFDumpPass);
-  registerPass(
-    "dwarfupdate", "update DWARF debug info sections", createDWARFUpdatePass);
   registerPass("duplicate-import-elimination",
                "removes duplicate imports",
                createDuplicateImportEliminationPass);

src/passes/passes.h

@@ -36,7 +36,6 @@ Pass* createDataFlowOptsPass();
 Pass* createDeadCodeEliminationPass();
 Pass* createDirectizePass();
 Pass* createDWARFDumpPass();
-Pass* createDWARFUpdatePass();
 Pass* createDuplicateImportEliminationPass();
 Pass* createDuplicateFunctionEliminationPass();
 Pass* createEmitTargetFeaturesPass();

src/wasm-binary.h

@@ -1044,6 +1044,19 @@ class WasmBinaryWriter {
 
   std::unique_ptr<ImportInfo> importInfo;
 
+  // General debugging info: map every instruction to its original position in
+  // the binary, relative to the beginning of the code section. This is similar
+  // to binaryLocations on Function objects, which are filled as we load the
+  // functions from the binary. Here we track them as we write, and then
+  // the combination of the two can be used to update DWARF info for the new
+  // locations of things.
+  BinaryLocationsMap binaryLocations;
+  size_t binaryLocationsSizeAtSectionStart;
+  // Track the expressions that we added for the current function being
+  // written, so that we can update those specific binary locations when
+  // the function is written out.
+  std::vector<Expression*> binaryLocationTrackedExpressionsForFunc;
+
   void prepare();
 };
 

src/wasm-debug.h

@@ -37,7 +37,7 @@ bool hasDWARFSections(const Module& wasm);
 void dumpDWARF(const Module& wasm);
 
 // Update the DWARF sections.
-void writeDWARFSections(Module& wasm);
+void writeDWARFSections(Module& wasm, const BinaryLocationsMap& newLocations);
 
 } // namespace Debug
 

src/wasm-stack.h

@@ -83,8 +83,16 @@ class BinaryInstWriter : public OverriddenVisitor<BinaryInstWriter> {
 public:
   BinaryInstWriter(WasmBinaryWriter& parent,
                    BufferWithRandomAccess& o,
-                   Function* func)
-    : parent(parent), o(o), func(func) {}
+                   Function* func,
+                   bool sourceMap)
+    : parent(parent), o(o), func(func), sourceMap(sourceMap) {}
+
+  void visit(Expression* curr) {
+    if (func && !sourceMap) {
+      parent.writeDebugLocation(curr, func);
+    }
+    OverriddenVisitor<BinaryInstWriter>::visit(curr);
+  }
 
   void visitBlock(Block* curr);
   void visitIf(If* curr);
@@ -144,6 +152,8 @@ class BinaryInstWriter : public OverriddenVisitor<BinaryInstWriter> {
   WasmBinaryWriter& parent;
   BufferWithRandomAccess& o;
   Function* func = nullptr;
+  bool sourceMap;
+
   std::vector<Name> breakStack;
 
   // type => number of locals of that type in the compact form
@@ -758,7 +768,7 @@ class BinaryenIRToBinaryWriter
                            Function* func = nullptr,
                            bool sourceMap = false)
     : BinaryenIRWriter<BinaryenIRToBinaryWriter>(func), parent(parent),
-      writer(parent, o, func), sourceMap(sourceMap) {}
+      writer(parent, o, func, sourceMap), sourceMap(sourceMap) {}
 
   void visit(Expression* curr) {
     BinaryenIRWriter<BinaryenIRToBinaryWriter>::visit(curr);
@@ -833,7 +843,7 @@ class StackIRToBinaryWriter {
   StackIRToBinaryWriter(WasmBinaryWriter& parent,
                         BufferWithRandomAccess& o,
                         Function* func)
-    : writer(parent, o, func), func(func) {}
+    : writer(parent, o, func, false /* sourceMap */), func(func) {}
 
   void write();
 

src/wasm.h

@@ -1129,7 +1129,10 @@ struct Importable {
 // Stack IR is a secondary IR to the main IR defined in this file (Binaryen
 // IR). See wasm-stack.h.
 class StackInst;
-typedef std::vector<StackInst*> StackIR;
+
+using StackIR = std::vector<StackInst*>;
+
+using BinaryLocationsMap = std::unordered_map<Expression*, uint32_t>;
 
 class Function : public Importable {
 public:
@@ -1178,7 +1181,7 @@ class Function : public Importable {
 
   // General debugging info: map every instruction to its original position in
   // the binary, relative to the beginning of the code section.
-  std::unordered_map<Expression*, uint32_t> binaryLocations;
+  BinaryLocationsMap binaryLocations;
 
   size_t getNumParams();
   size_t getNumVars();

src/wasm/wasm-binary.cpp

@@ -72,6 +72,14 @@ void WasmBinaryWriter::write() {
     writeSourceMapEpilog();
   }
 
+#ifdef BUILD_LLVM_DWARF
+  // Update DWARF user sections after writing the data referred to by them
+  // (function bodies), and before writing the user sections themselves.
+  if (Debug::hasDWARFSections(*wasm)) {
+    Debug::writeDWARFSections(*wasm, binaryLocations);
+  }
+#endif
+
   writeLateUserSections();
   writeFeaturesSection();
 
@@ -109,29 +117,52 @@ template<typename T> int32_t WasmBinaryWriter::startSection(T code) {
   if (sourceMap) {
     sourceMapLocationsSizeAtSectionStart = sourceMapLocations.size();
   }
+  binaryLocationsSizeAtSectionStart = binaryLocations.size();
   return writeU32LEBPlaceholder(); // section size to be filled in later
 }
 
 void WasmBinaryWriter::finishSection(int32_t start) {
   // section size does not include the reserved bytes of the size field itself
   int32_t size = o.size() - start - MaxLEB32Bytes;
   auto sizeFieldSize = o.writeAt(start, U32LEB(size));
-  if (sizeFieldSize != MaxLEB32Bytes) {
+  // We can move things back if the actual LEB for the size doesn't use the
+  // maximum 5 bytes. In that case we need to adjust offsets after we move
+  // things backwards.
+  auto adjustmentForLEBShrinking = MaxLEB32Bytes - sizeFieldSize;
+  if (adjustmentForLEBShrinking) {
     // we can save some room, nice
     assert(sizeFieldSize < MaxLEB32Bytes);
     std::move(&o[start] + MaxLEB32Bytes,
               &o[start] + MaxLEB32Bytes + size,
               &o[start] + sizeFieldSize);
-    auto adjustment = MaxLEB32Bytes - sizeFieldSize;
-    o.resize(o.size() - adjustment);
+    o.resize(o.size() - adjustmentForLEBShrinking);
     if (sourceMap) {
       for (auto i = sourceMapLocationsSizeAtSectionStart;
            i < sourceMapLocations.size();
            ++i) {
-        sourceMapLocations[i].first -= adjustment;
+        sourceMapLocations[i].first -= adjustmentForLEBShrinking;
       }
     }
   }
+
+  if (binaryLocationsSizeAtSectionStart != binaryLocations.size()) {
+    // We added the binary locations, adjust them: they must be relative
+    // to the code section.
+    assert(binaryLocationsSizeAtSectionStart == 0);
+    // The section type byte is right before the LEB for the size; we want
+    // offsets that are relative to the body, which is after that section type
+    // byte and the the size LEB.
+    auto body = start + sizeFieldSize;
+    for (auto& pair : binaryLocations) {
+      // Offsets are relative to the body of the code section: after the
+      // section type byte and the size.
+      // Everything was moved by the adjustment, track that. After this,
+      // we are at the right absolute address.
+      pair.second -= adjustmentForLEBShrinking;
+      // We are relative to the section start.
+      pair.second -= body;
+    }
+  }
 }
 
 int32_t
@@ -266,6 +297,7 @@ void WasmBinaryWriter::writeFunctions() {
   auto start = startSection(BinaryConsts::Section::Code);
   o << U32LEB(importInfo->getNumDefinedFunctions());
   ModuleUtils::iterDefinedFunctions(*wasm, [&](Function* func) {
+    assert(binaryLocationTrackedExpressionsForFunc.empty());
     size_t sourceMapLocationsSizeAtFunctionStart = sourceMapLocations.size();
     BYN_TRACE("write one at" << o.size() << std::endl);
     size_t sizePos = writeU32LEBPlaceholder();
@@ -284,22 +316,31 @@ void WasmBinaryWriter::writeFunctions() {
     BYN_TRACE("body size: " << size << ", writing at " << sizePos
                             << ", next starts at " << o.size() << "\n");
     auto sizeFieldSize = o.writeAt(sizePos, U32LEB(size));
-    if (sizeFieldSize != MaxLEB32Bytes) {
+    // We can move things back if the actual LEB for the size doesn't use the
+    // maximum 5 bytes. In that case we need to adjust offsets after we move
+    // things backwards.
+    auto adjustmentForLEBShrinking = MaxLEB32Bytes - sizeFieldSize;
+    if (adjustmentForLEBShrinking) {
       // we can save some room, nice
       assert(sizeFieldSize < MaxLEB32Bytes);
       std::move(&o[start], &o[start] + size, &o[sizePos] + sizeFieldSize);
-      auto adjustment = MaxLEB32Bytes - sizeFieldSize;
-      o.resize(o.size() - adjustment);
+      o.resize(o.size() - adjustmentForLEBShrinking);
       if (sourceMap) {
         for (auto i = sourceMapLocationsSizeAtFunctionStart;
              i < sourceMapLocations.size();
              ++i) {
-          sourceMapLocations[i].first -= adjustment;
+          sourceMapLocations[i].first -= adjustmentForLEBShrinking;
         }
       }
+      for (auto* curr : binaryLocationTrackedExpressionsForFunc) {
+        // We added the binary locations, adjust them: they must be relative
+        // to the code section.
+        binaryLocations[curr] -= adjustmentForLEBShrinking;
+      }
     }
     tableOfContents.functionBodies.emplace_back(
       func->name, sizePos + sizeFieldSize, size);
+    binaryLocationTrackedExpressionsForFunc.clear();
   });
   finishSection(start);
 }
@@ -649,10 +690,19 @@ void WasmBinaryWriter::writeDebugLocation(const Function::DebugLocation& loc) {
 }
 
 void WasmBinaryWriter::writeDebugLocation(Expression* curr, Function* func) {
-  auto& debugLocations = func->debugLocations;
-  auto iter = debugLocations.find(curr);
-  if (iter != debugLocations.end()) {
-    writeDebugLocation(iter->second);
+  if (sourceMap) {
+    auto& debugLocations = func->debugLocations;
+    auto iter = debugLocations.find(curr);
+    if (iter != debugLocations.end()) {
+      writeDebugLocation(iter->second);
+    }
+  }
+  // TODO: remove source map debugging support and refactor this method
+  // to something that directly thinks about DWARF, instead of indirectly
+  // looking at func->binaryLocations as a proxy for that etc.
+  if (func && !func->binaryLocations.empty()) {
+    binaryLocations[curr] = o.size();
+    binaryLocationTrackedExpressionsForFunc.push_back(curr);
   }
 }
 
@@ -809,6 +859,9 @@ void WasmBinaryBuilder::read() {
         readFunctionSignatures();
         break;
       case BinaryConsts::Section::Code:
+        if (DWARF) {
+          codeSectionLocation = pos;
+        }
         readFunctions();
         break;
       case BinaryConsts::Section::Export:
@@ -1288,9 +1341,6 @@ void WasmBinaryBuilder::readFunctionSignatures() {
 
 void WasmBinaryBuilder::readFunctions() {
   BYN_TRACE("== readFunctions\n");
-  if (DWARF) {
-    codeSectionLocation = pos;
-  }
   size_t total = getU32LEB();
   if (total != functionSignatures.size()) {
     throwError("invalid function section size, must equal types");