asm2wasm debuginfo #895

kripken · 2017-02-03T06:03:08Z

Users have complained that debugging wasm is hard, and it'll be a while til we have proper source maps or such. Meanwhile, this pr might help a little, it makes asm2wasm use debug info when present, and emit it as comments in the wast. For example, hello world might look like this:

  (func $_main (result i32)
    ;; tests/hello_world.c:4
    (drop
      (call $_printf
        (i32.const 1144)
        (i32.const 1160)
      )
    )
    ;; tests/hello_world.c:5
    (return
      (i32.const 0)
    )
  )

Technically, the debug info is translated into intrinsic calls. This is for efficiency, so that they incur no overhead when not using it (so we don't make each node have a debug info field or anything like that). Then we compile (and possibly optimize), and then we translate those intrinsics into annotations on the nodes, which are a map on the side per function of node => annotation. Then the printer just prints the annotation with the node, if there is one.

kripken · 2017-02-03T06:04:30Z

Some error on the bots and locally other.test_binaryen_names fails - posting to remind myself to look at this tomorrow.

ignore --debuginfo if not emitting text, as wasm binaries don't support that yet

…binary, all we can do is the Names section

kripken · 2017-02-03T21:25:38Z

This should be good now, if anyone wants to review.

jgravelle-google · 2017-02-04T00:51:18Z

src/asm2wasm.h

+        if (out + 100 >= end) {
+          Fatal() << "error in handling debug info";
+        }
+        if (input[0] == '/' && input[1] == '/' && input[2] == '@' && input[3] == 'l' && input[4] == 'i' && input[5] == 'n' && input[6] == 'e') {


What about startsWith(input, "//@line") ? Providing startsWith in a utility header.

Fixing. I worry it will be less efficient, but I guess a modern compiler should be able to do this well...

jgravelle-google · 2017-02-04T00:57:56Z

src/asm2wasm.h

+          *out++ = *input++;
+          *out++ = *input++;
+          *out++ = *input++;
+          *out++ = *input++;


jgravelle-google · 2017-02-04T01:48:22Z

src/asm2wasm.h

+      // before it - this is guaranteed to be correct without opts,
+      // and is usually decently accurate with them.
+      auto size = strlen(input);
+      auto upperBound = Index(size * 1.25) + 100;


Magic numbers are magic :)

Does this 100 match the 100 a few lines down?
Would be good to replace these with named constants for clarity.

Yeah, good point, fixing.

jgravelle-google · 2017-02-04T01:51:56Z

src/asm2wasm.h

@@ -565,6 +649,16 @@ class Asm2WasmBuilder {
  }

  Function* processFunction(Ref ast);
+
+public:
+  CallImport* isDebugInfo(Expression* curr) {


is___ makes me thing "boolean". Better name would be just debugInfo, which can still be used in a boolean context.

Good point, yeah. Changing to checkDebugInfo which is consistent with other stuff in wasm.h.

jgravelle-google · 2017-02-04T01:54:53Z

src/mixed_arena.h

@@ -19,6 +19,7 @@

 #include <atomic>
 #include <cassert>
+#include <cstdlib>


Seems unused

It's needed for abort, this just wasn't noticed before because other locations happened to have the include anyhow, I think.

Ah, makes sense

kripken · 2017-02-06T22:15:57Z

Thanks for the feedback, added a commit with fixes.

jgravelle-google · 2017-02-06T23:10:48Z

src/mixed_arena.h

@@ -19,6 +19,7 @@

 #include <atomic>
 #include <cassert>
+#include <cstdlib>


Ah, makes sense

jgravelle-google · 2017-02-06T23:16:23Z

src/asm2wasm.h

+      std::string DEBUGINFO_INTRINSIC = EMSCRIPTEN_DEBUGINFO.str;
+      auto DEBUGINFO_INTRINSIC_SIZE = DEBUGINFO_INTRINSIC.size();
+      bool seenUseAsm = false;
+      auto startsWith = [&](const char *s) {


I would make this a top-level function as opposed to a lambda

when reading this code it can be assumed that startsWith exists elsewhere, so it doesn't interrupt the flow of reading this function

the way its currently implemented a better name would be inputAtTheCurrentPositionStartsWith("foo"). I feel startsWith(input, "foo") is more explicit at the call site and requires less context for the reader to keep in their head

Both should be inlineable by the compiler so it should be equivalent to the unrolled version, either way.

Fair points, but which toplevel? Of the object? Or the entire file?

In my head it's a top-level function that requires no state, so of the entire file. In theory it could be put in a utils header and shared.

ok, refactored to top level in the file

jgravelle-google · 2017-02-06T23:20:50Z

src/asm2wasm.h

+          out += line.size();
+          *out++ = ')';
+          *out++ = ';';
+        } else if (!seenUseAsm && (startsWith("asm'") || startsWith("asm\""))) {


This could theoretically be optimized by the compiler to be equivalent to what came before, but if (!seenUseAsm && startsWith(input, "asm") && (input[3] == '\'' || input[3] == '"')) is a possible rewrite that should almost-certainly be the same, while being I think equally legible. (I feel ambivalent about this either way though)

jgravelle-google · 2017-02-06T23:24:00Z

src/asm2wasm.h

+      // before it - this is guaranteed to be correct without opts,
+      // and is usually decently accurate with them.
+      const auto SCALE_FACTOR = 1.25; // an upper bound on how much more space we need as a multiple of the original
+      const auto ADD_FACTOR = 100; // an upper bound on how much we write for each debug info element itself


These comments are pretty helpful, thanks!

jgravelle-google · 2017-02-07T16:25:09Z

src/asm2wasm.h

+static bool startsWith(const char* string, const char *prefix) {
+  while (1) {
+    if (*prefix == 0) return true;
+    if (*string++ != *prefix++) return false;


Should also probably return false if *string == 0 for completeness, e.g. startsWith("foo", "foobar")

Good point, fixing.

jgravelle-google

Modulo nits, lgtm!

jgravelle-google · 2017-02-07T16:27:20Z

src/asm2wasm.h

+        } else if (!seenUseAsm && (startsWith(input, "asm'") || startsWith(input, "asm\""))) {
+          // end of  "use asm"  or  "almost asm"
+          seenUseAsm = true;
+          memcpy(out, input, 5);


Probably worth pulling the 5s into a constant?
Actually yeah it's not obvious why 5 and not 4, presumably consuming asm\'\n?

Heh, it's actually the semicolon at the end, "use asm";. I'll clarify.

jgravelle-google · 2017-02-07T16:32:21Z

src/asm2wasm.h

+          strcpy(out, line.c_str());
+          out += line.size();
+          *out++ = ')';
+          *out++ = ';';


Nothing to do here just a thought/comment/observation:
This interleaving section of strcpy and *out++ = char is pretty verbose and I wish there was a cleaner/more compact way to do it.
Something like outStream << DEBUGINFO_INTRINSIC << '(' << fileIndex << ',' << line << ");", though that would require a very different model of how this is done and probably isn't worth it?

Yeah, might be a better way. The code is not expected to change much if at all, though, so I think it's maintainable as it is. If it were evolving code definitely it would need to be refactored.

parse file/line comments in asm.js into debug intrinsics

e5fe58e

kripken mentioned this pull request Feb 3, 2017

asm2wasm debuginfo emscripten-core/emscripten#4917

Merged

kripken force-pushed the asm2wasm-debuginfo branch 3 times, most recently from d46b672 to 23c5752 Compare February 3, 2017 18:46

convert debug intrinsics into annotations, and print them

4c1e369

ignore --debuginfo if not emitting text, as wasm binaries don't support that yet

kripken force-pushed the asm2wasm-debuginfo branch from 23c5752 to 55b7289 Compare February 3, 2017 19:51

kripken added 2 commits February 3, 2017 13:07

emit full debug info when -g and emitting text; when -g and emitting …

7d800b2

…binary, all we can do is the Names section

update wasm.js

3e75a78

kripken force-pushed the asm2wasm-debuginfo branch from ff420e1 to 3e75a78 Compare February 3, 2017 21:07

jgravelle-google reviewed Feb 4, 2017

View reviewed changes

address review comments

f372841

jgravelle-google reviewed Feb 6, 2017

View reviewed changes

refactor startsWith

85070ec

jgravelle-google reviewed Feb 7, 2017

View reviewed changes

jgravelle-google approved these changes Feb 7, 2017

View reviewed changes

address more review comments

e7775bc

kripken merged commit 76c8f82 into master Feb 7, 2017

kripken deleted the asm2wasm-debuginfo branch February 7, 2017 19:25

yurydelendik mentioned this pull request May 19, 2017

Exporting/importing debug location information from .wast/.asm.js/.s formats #1017

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

asm2wasm debuginfo #895

asm2wasm debuginfo #895

kripken commented Feb 3, 2017

kripken commented Feb 3, 2017

kripken commented Feb 3, 2017

jgravelle-google Feb 4, 2017

kripken Feb 6, 2017

jgravelle-google Feb 4, 2017

kripken Feb 6, 2017

jgravelle-google Feb 4, 2017

kripken Feb 6, 2017

jgravelle-google Feb 4, 2017

kripken Feb 6, 2017

jgravelle-google Feb 4, 2017

kripken Feb 6, 2017

jgravelle-google Feb 6, 2017

kripken commented Feb 6, 2017

jgravelle-google Feb 6, 2017

jgravelle-google Feb 6, 2017

kripken Feb 6, 2017

jgravelle-google Feb 6, 2017

kripken Feb 7, 2017

jgravelle-google Feb 6, 2017

jgravelle-google Feb 6, 2017

jgravelle-google Feb 7, 2017

kripken Feb 7, 2017

jgravelle-google left a comment

jgravelle-google Feb 7, 2017

kripken Feb 7, 2017

jgravelle-google Feb 7, 2017

kripken Feb 7, 2017

asm2wasm debuginfo #895

asm2wasm debuginfo #895

Conversation

kripken commented Feb 3, 2017

kripken commented Feb 3, 2017

kripken commented Feb 3, 2017

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 Feb 6, 2017

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

jgravelle-google 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