DWARF: Fix handling of the end of control flow instructions (#3288)

Previously when we processed a block for example, we'd do this:

;; start is here
(block (result type)
;; end is here
  .. contents ..
)
;; end delimiter is here

Not how this represents the block's start and end as the "header", and
uses an extra delimiter to mark the end.

I think this is wrong, and was an attempt to handle some offsets from
LLVM that otherwise made no sense, ones at the end of the "header".
But it turns out that this makes us completely incorrect on some things
where there is a low/high pc pair, and we need to understand that the
end of a block is at the end opcode at the very end, and not the end of
the header. This PR changes us to do that, i.e.

;; start is here
(block (result type)
  .. contents ..
)
;; end is here

This fixes a testcase already in the test suite,

test/passes/fib_nonzero-low-pc_dwarf.bin.txt

where you can see that lexical block now has a valid value for the end, and
not a 0 (the proper scope extends all the way to the end of the big block in
that function, and is now the same in the DWARF before and after we
process it). test/passes/fannkuch3_dwarf.bin.txt is also improved by
this.

To implement this, this removes the BinaryLocations::End delimeter. After
this we just need one type of delimiter actually, but I didn't refactor that any
more to keep this PR small (see TODO).

This removes an assertion in writeDebugLocationEnd() that is no longer
valid: the assert ensures that we wrote an end only if there was a 0 for
the end, but for a control flow structure, we write the end of the "header"
automatically like for any expression, and then overwrite it later when we
finish writing the children and the end marker. We could in theory special-case
control flow structures to avoid the first write, but it would add more complexity.

This uncovered what appears to be a possible bug in our debug_line
handling, see test/passes/fannkuch3_manyopts_dwarf.bin.txt. That needs
to be looked into more, but I suspect that was invalid info from when we
looked at the end of the "header" of control flow structures. Note that there
was one definite bug uncovered here, fixed by the extra

} else if (locationUpdater.hasOldExprEnd(oldAddr)) {

that is added here, which was definitely a bug.

1 files changed, 2 insertions, 2 deletions

diff --git a/src/wasm/wasm-stack.cpp b/src/wasm/wasm-stack.cpp
index 2ec156eea..1d290f53c 100644
--- a/src/wasm/wasm-stack.cpp
+++ b/src/wasm/wasm-stack.cpp
@@ -1880,10 +1880,10 @@ void BinaryInstWriter::visitArrayLen(ArrayLen* curr) {
 void BinaryInstWriter::emitScopeEnd(Expression* curr) {
   assert(!breakStack.empty());
   breakStack.pop_back();
+  o << int8_t(BinaryConsts::End);
   if (func && !sourceMap) {
-    parent.writeExtraDebugLocation(curr, func, BinaryLocations::End);
+    parent.writeDebugLocationEnd(curr, func);
   }
-  o << int8_t(BinaryConsts::End);
 }
 
 void BinaryInstWriter::emitFunctionEnd() { o << int8_t(BinaryConsts::End); }