DWARF debug line updating #2545

kripken · 2019-12-20T22:30:19Z

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

dschuff · 2019-12-20T22:51:16Z

which "special" debug line opcodes are you referring to?

dschuff

haven't looked at LineState yet...

dschuff · 2019-12-20T23:02:11Z

src/wasm-stack.h

@@ -144,6 +152,8 @@ class BinaryInstWriter : public OverriddenVisitor<BinaryInstWriter> {
  WasmBinaryWriter& parent;
  BufferWithRandomAccess& o;
  Function* func = nullptr;
+  bool sourceMap;


is this just use for source maps for for dwarf debug_line too?

This is just for source maps - when source maps are enabled, we don't update debug info in the way that DWARF needs, and vice versa. So we need to know if source maps are used (previously there was just one way to do debug locations).

dschuff · 2019-12-20T23:06:41Z

src/wasm/wasm-binary.cpp

  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) {
+  auto adjustment = MaxLEB32Bytes - sizeFieldSize;


now that adjustment isn't quite as local, maybe it should be called something more descriptive such as sectionHeaderSizeAdjustment (or something better/shorter if you can think of something)

Good idea, renaming to adjustmentForLEBShrinking, how is that?

it's just LEB shrinking of the header though, right?

Yes, just the header's LEB (here, of the section - the same happens in each function as they also have an LEB size).

src/wasm/wasm-binary.cpp

dschuff · 2019-12-20T23:09:35Z

src/wasm/wasm-binary.cpp

+    // We added the binary locations, adjust them: they must be relative
+    // to the code section.
+    assert(binaryLocationsSizeAtSectionStart == 0);
+    // The actual section start is right before the LEB for the size; we want


this means that the "actual" section start is at the start of the size, but not at the section type? Is that common in other tools/elsewehre in Binaryen? It seems really counterintuitive to me. I would think of the section as starting after the "header" which is the type + size. It's confusing that the payload includes the name, but that's a separate issue?

That comment is confusing, yeah, I'll reword. Basically we want to have offsets relative to the "body", which is after the section type byte and the size LEB.

src/wasm/wasm-binary.cpp

dschuff · 2019-12-20T23:19:15Z

src/wasm/wasm-debug.cpp


-void writeDWARFSections(Module& wasm) {
+// Represents the state when parsing a line table.


where in LLVM is it that does this same logic?

I think this is the main place:

binaryen/third_party/llvm-project/DWARFDebugLine.cpp

Line 531 in bd4cac9

Error DWARFDebugLine::LineTable::parse(

kripken · 2019-12-20T23:47:47Z

which "special" debug line opcodes are you referring to?

Oh, the debug line opcodes that update both the line + the address + end the sequence, all in one byte. Here is the LLVM code for them:

binaryen/third_party/llvm-project/DWARFDebugLine.cpp

Line 836 in bd4cac9

// Special Opcodes

src/wasm/wasm-debug.cpp

dschuff · 2019-12-20T23:33:41Z

src/wasm/wasm-debug.cpp

+  bool prologueEnd = false;
+  bool epilogueBegin = false;
+
+  LineState() = default;


can this be deleted?

I think it's possible, by avoiding newAddrInfo[newAddr] accesses (which I guess makes the hash map code consider adding an empty element). Using .at, .emplace allows this to work without that. That's safer I guess, although slightly less readable perhaps, but also useful due to other reasons in the discussion below.

dschuff · 2019-12-20T23:35:35Z

src/wasm/wasm-debug.cpp

-void writeDWARFSections(Module& wasm) {
+// Represents the state when parsing a line table.
+struct LineState {
+  uint32_t addr = 0;


could be Address?

I think Address is a wasm address, so something in the wasm memory. This is an offset in the wasm code section. I think that has a 4GB limit in wasm, so uint32 seems reasonable?

I guess so... until wasm64 starts allowing >4G code as well as data...

dschuff · 2019-12-20T23:55:20Z

src/wasm/wasm-debug.cpp

+  }
+
+  // Given an old state, emit the diff from it to this state into a new line
+  // table.


should this be "into a new LineState" or "into a new line" or "into a new line table entry"

The latter. I'll add that and clarify more.

src/wasm/wasm-debug.cpp

dschuff · 2019-12-21T00:07:34Z

src/wasm/wasm-debug.cpp

+          if (iter != newLocations.end()) {
+            uint32_t newAddr = iter->second;
+            newAddrs.push_back(newAddr);
+            auto& updatedState = newAddrInfo[newAddr] = state;


this is a pretty ugly compound assignment since the intermediate assignment is a copy but the final one is actually taking a reference.

Could you do auto& updatedState = newAddrInfo.insert({newAddr, state}).first? Also a bit ugly but at least it's maybe more clear all the stuff that's going on, and it's still one line.

Refactoring this to also help getting rid of that default constructor you didn't like.

src/wasm/wasm-debug.cpp

dschuff · 2019-12-21T00:29:08Z

src/wasm/wasm-debug.cpp

+  bool epilogueBegin = false;
+
+  LineState() = default;
+  LineState(const LineState& other) = default;


I guess if we have an explicitly-defaulted copy constructor we should have an explicitly-defaulted assignment operator too?

It wouldn't be used yet - not sure what the benefit is to adding one at this point? Is it easier to read for you that way?

It was used, in that compound assignment I complained about. More generally it's just a rule of thumb that whatever you do with the copy constructor (default, delete, etc) you should also do with the assignment operator.

Ok, sounds good, added.

dschuff · 2019-12-21T01:10:55Z

src/wasm/wasm-debug.cpp

-void writeDWARFSections(Module& wasm) {
+// Represents the state when parsing a line table.
+struct LineState {
+  uint32_t addr = 0;


I guess so... until wasm64 starts allowing >4G code as well as data...

dschuff · 2019-12-21T01:18:08Z

src/wasm/wasm-debug.cpp

+  bool epilogueBegin = false;
+
+  LineState() = default;
+  LineState(const LineState& other) = default;


It was used, in that compound assignment I complained about. More generally it's just a rule of thumb that whatever you do with the copy constructor (default, delete, etc) you should also do with the assignment operator.

kripken and others added 16 commits December 19, 2019 15:58

Fix binary offset tracking and update DWARF debug line numbers

c7bc810

tests [ci skip]

be73634

fix

4db874b

fixes

46b20e7

fixes [ci skip]

e77b028

test file [ci skip]

0194159

better

28dc2e0

style

b6edcae

this isn't python

2b4ce9e

fix [ci skip]

96175a6

correct offsets!

31f3e4d

cleanup

566cc6f

test [ci skip]

8b75591

fix ignoring

c5e7c39

proper test [ci skip]

3358273

fix

012a5d9

kripken requested a review from dschuff December 20, 2019 22:30

kripken mentioned this pull request Dec 20, 2019

Support debug info in Binaryen #2400

Closed

dschuff reviewed Dec 20, 2019

View reviewed changes

kripken added 2 commits December 20, 2019 15:54

review feedback

7ce32a8

Another comment

fb506f6

dschuff reviewed Dec 21, 2019

View reviewed changes

kripken added 2 commits December 20, 2019 16:50

safer

dd105e0

More feedback

fb69a95

dschuff approved these changes Dec 21, 2019

View reviewed changes

add assignment operator

aa2a68e

kripken merged commit 3f6fd58 into master Dec 21, 2019

kripken deleted the binloc3 branch December 21, 2019 03:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DWARF debug line updating #2545

DWARF debug line updating #2545

kripken commented Dec 20, 2019

dschuff commented Dec 20, 2019

dschuff left a comment

dschuff Dec 20, 2019

kripken Dec 20, 2019

dschuff Dec 20, 2019

kripken Dec 20, 2019

dschuff Dec 21, 2019

kripken Dec 21, 2019

dschuff Dec 20, 2019

kripken Dec 20, 2019

dschuff Dec 20, 2019

kripken Dec 20, 2019

kripken commented Dec 20, 2019

dschuff Dec 20, 2019

kripken Dec 21, 2019

dschuff Dec 20, 2019

kripken Dec 21, 2019

dschuff Dec 21, 2019

dschuff Dec 20, 2019

kripken Dec 21, 2019

dschuff Dec 21, 2019

dschuff Dec 21, 2019

kripken Dec 21, 2019

dschuff Dec 21, 2019

kripken Dec 21, 2019

dschuff Dec 21, 2019

kripken Dec 21, 2019

dschuff Dec 21, 2019

dschuff Dec 21, 2019


		void writeDWARFSections(Module& wasm) {
		// Represents the state when parsing a line table.

DWARF debug line updating #2545

DWARF debug line updating #2545

Conversation

kripken commented Dec 20, 2019

dschuff commented Dec 20, 2019

dschuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Dec 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment