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

yurydelendik · 2017-05-19T19:57:36Z

This change allows (based on @dschuff 's debuginfo branch):

To read stored in .asm.js file debug locations into AST
To export ~~.txtmap~~.map file along with .wasm binary file export
To read ~~.txtmap~~.map file (when .wasm read) information into AST
To read .s file information into AST (see Parse debug information such as files and locations from .s files. #347 and Allows parsing debug information in the .S files #544)
Changes wast comment format (see asm2wasm debuginfo #895) ;; file.ext:line to ;;@ file.ext:line:column

yurydelendik · 2017-05-19T20:03:36Z

kripken · 2017-05-19T20:20:47Z

src/passes/Print.cpp

@@ -599,6 +603,7 @@ struct PrintSExpression : public Visitor<PrintSExpression> {
  }
  void visitFunction(Function *curr) {
    currFunction = curr;
+    lastPrintedLocation = {0, 0, 0};


i think space after { and before } is what we have more commonly in the codebase.

kripken · 2017-05-19T20:22:07Z

src/tools/asm2wasm.cpp

+      .add("--binarymap-file", "-bm", "Emit binary map (if using binary output) to the specified file",
+           Options::Arguments::One,
+           [&binaryMapFile](Options *o, const std::string &argument) { binaryMapFile = argument; })
+      .add("--binarymap-url", "-bu", "Use specified string as binary map URL",


what does this do?

This parameter defines the sourceMappingURL property that will be written in the .wasm custom section.

kripken · 2017-05-19T20:23:30Z

src/tools/asm2wasm.cpp

@@ -132,8 +140,7 @@ int main(int argc, const char *argv[]) {
  }

  Asm2WasmPreProcessor pre;
-  // wasm binaries can contain a names section, but not full debug info
-  pre.debugInfo = passOptions.debugInfo && !emitBinary;
+  pre.debugInfo = passOptions.debugInfo;


if we are emitting binary, and not emitting a binary map, we can drop the debug info, I think? I.e. replace !emitBinary with something like (!emitBinary || !binaryMapFile)

kripken · 2017-05-19T20:25:16Z

src/tools/asm2wasm.cpp

@@ -200,7 +207,11 @@ int main(int argc, const char *argv[]) {
  writer.setDebugInfo(passOptions.debugInfo);
  writer.setSymbolMap(symbolMap);
  writer.setBinary(emitBinary);
-  writer.write(wasm, options.extra["output"]);
+  if (emitBinary && binaryMapFile.size()) {
+    writer.writeBinary(wasm, options.extra["output"], binaryMapFile, binaryMapUrl);


i think a more consistent api might be to have, like we have setBinary now (used a few lines above), setBinaryMapFile and setBinaryMapURL methods that we call. then we can avoid calling writeBinary here.

kripken · 2017-05-19T20:25:43Z

src/tools/wasm-as.cpp

@@ -86,13 +94,24 @@ int main(int argc, const char *argv[]) {
  if (options.debug) std::cerr << "binarification..." << std::endl;
  BufferWithRandomAccess buffer(options.debug);
  WasmBinaryWriter writer(&wasm, buffer, options.debug);
-  writer.setDebugInfo(debugInfo);
+  writer.setNamesSection(debugInfo);
+  std::ofstream* binaryMapStream = nullptr;


std::unique_ptr

kripken · 2017-05-19T20:27:11Z

src/wasm-binary.h

+      if (iter != debugLocations.end() && iter->second != lastDebugLocation) {
+        lastDebugLocation = iter->second;
+        auto fileName = wasm->debugInfoFileNames[iter->second.fileIndex];
+        *binaryMap << o.size() << ":" << fileName << ":" <<iter->second.lineNumber << ":" <<iter->second.columnNumber << '\n';


spaces before and after each <<

kripken · 2017-05-19T20:28:08Z

src/wasm.h

-    uint32_t fileIndex, lineNumber;
+    uint32_t fileIndex, lineNumber, columnNumber;
+    bool operator==(const DebugLocation& other) { return fileIndex == other.fileIndex && lineNumber == other.lineNumber && columnNumber == other.columnNumber; }
+    bool operator!=(const DebugLocation& other) { return !this->operator==(other); }


how about using the operator, i.e. !(other == *this)?

kripken · 2017-05-19T20:30:04Z

General questions, maybe I don't understand how debug info is meant to work yet:

What is the URL?
Looks like there is both code to write out a separate binary map file, but also read and write inside the binary?

yurydelendik · 2017-05-19T20:48:00Z

What is the URL?

The URL is the final JavaScript JSON source maps resource (as defined in the WebAssembly/design#1051) and intended to be read (URL and then source maps) by devtools.
.txtmap-file will be further transformed into/from JSON source maps as defined by the design PR.

Looks like there is both code to write out a separate binary map file, but also read and write inside the binary?

At the moment .txtmap is used to store debug information, but final "destination" is JSON map file. The .wasm and .txtmap (later .map) file go side-by-side (similar to JS source maps). The .wasm binary file does not contain any debug location information, only the sourceMappingURL (which points to .map). To import the location information, the .txtmap (later .map) must be read at the same time as .wasm by wasm-dis to place that in AST.

kripken · 2017-05-19T22:24:50Z

Thanks, but what does "later map" mean in all those cases? Is that an expected spec change, or something happening now in the toolchain?

yurydelendik · 2017-05-19T23:00:47Z

Thanks, but what does "later map" mean in all those cases? Is that an expected spec change, or something happening now in the toolchain?

For emscripten, there is emscripten-core/emscripten@e8dd1c8 which is planning to use regular tools/source-maps/sourcemapper.js, however the design PR recommends to use columns instead of lines. We can further modify https://github.com/kripken/emscripten/blob/master/tools/source-maps/sourcemapper.js#L130 to use byteoffset as a column. For prototyping, I'm using txt2map4wasm package (https://github.com/yurydelendik/txt2map4wasm).

yurydelendik · 2017-05-19T23:05:05Z

@kripken Would you recommend to add JavaScript source map generation to this PR?

kripken · 2017-05-22T16:54:57Z

Ok, I think I see, so this emits some map file that is later translated to a full source map by the sourcemapper tool in emscripten? And you're asking if we should do the full translation here?

If that translation is trivial, it might make sense to duplicate that functionality here, since then the code here would be useful by itself. But I'm not sure how simple it is, what do you think?

yurydelendik · 2017-05-22T21:36:08Z

But I'm not sure how simple it is, what do you think?

About 200 SLOC, I'll submit that shortly

yurydelendik · 2017-05-23T13:30:04Z

Now asm2wasm and friends are exporting valid JS source map directly. For sake of simplicity of this PR and sake of re-import (for wasm-dis), I limited it to the following format:

{"version":3,"sources":[…source file names…],"names":[],"mappings":"…VLQ encoded entries…"}

notice that the "sources" field must precede the "mappings" field in the JSON.

kripken · 2017-05-23T18:32:13Z

check.py

+        # verify debug info
+        if 'debugInfo' in asm:
+          jsmap = 'a.wasm.map'
+          cmd += ['--binarymap-file', jsmap,


since the tools now emit the source map directly, do we need binarymap here?

the file is still a standalone file, I just changed its output format -- binary wasm just contains the reference

so --binarymap-file emits a source map? what i am asking is, should we change the parameter names to --source-map or something similar, if they just emit a source map?

or am I missing something here? is there still a binary file, with a "reference"? if so where is that documented, i don't think i see it in t he design pr?

is there still a binary file, with a "reference"? if so where is that documented, i don't think i see it in t he design pr?

https://github.com/WebAssembly/design/pull/1051/files#diff-8e85308ab5cc1e83e91ef59233648be2R219

"For wasm, a custom section named "sourceMappingURL" contains the URL."

what i am asking is, should we change the parameter names to --source-map or something similar

I can change that, yes. For JSON file name it will be --source-map, and for url (embedded in the wasm) --source-map-url. Is it correct?

yes, those names sound good.

to make sure i understand, the current state is now that we emit just 2 files, the wasm binary (which has a section that has the URL) and the JSON source map file (which that URL should point to)?

That is correct. If --source-map is specified, asm2wasm and wasm-as emit additional JSON source map file. If --source-map-url is specified, the optional custom section section "sourceMappingURL" will be added to the wasm file.

If --source-map is specified for wasm-dis, this JSON source map file is used to read location information and to add these to the wast output.

kripken · 2017-05-23T23:39:49Z

check.py

+          run_command(cmd)
+          if not os.path.isfile(jsmap):
+            fail_with_error('Debug info map not created: %s' % jsmap)
+          with open(wasm + '.map', 'rb') as expected:


looks like this uses some new test output files in the test dir. please also update auto_update_tests.py which should generate all the test output files in the test folder

kripken · 2017-05-23T23:41:37Z

src/s2wasm.h

@@ -665,22 +668,31 @@ class S2WasmBuilder {

    mustMatch(":");

+    Function::DebugLocation debugLocation = {0,0,0};


here and in a few more places, spaces after { and before } would be more consistent with the rest of the code

kripken · 2017-05-23T23:44:27Z

src/tools/wasm-as.cpp

+           [&sourceMapFilename](Options *o, const std::string &argument) { sourceMapFilename = argument; })
+      .add("--source-map-url", "-su", "Use specified string as source map URL",
+           Options::Arguments::One,
+           [&sourceMapUrl](Options *o, const std::string &argument) { sourceMapUrl = argument; })


these are duplicated in two files. i am currently getting rid of most of the existing duplication in #1023. it would be good to do the same thing for these options, however, i understand if you want to land this first and leave the refactoring to followup work.

I would rather move this out of scope this PR -- --source-map and --source-map-url are not really optimization options and have different meaning in some cases, e.g. for asm2wasm --source-map means output file, but for wasm-dis -- input file.

Yeah, good point, we'll need more refactoring here. Definitely let's leave for later.

kripken · 2017-05-23T23:46:02Z

src/tools/wasm-as.cpp

@@ -86,13 +94,22 @@ int main(int argc, const char *argv[]) {
  if (options.debug) std::cerr << "binarification..." << std::endl;
  BufferWithRandomAccess buffer(options.debug);
  WasmBinaryWriter writer(&wasm, buffer, options.debug);
-  writer.setDebugInfo(debugInfo);
+  writer.setNamesSection(debugInfo);


this looks weird, it used to be the same name, but now it is different. perhaps add a comment, "if debug info is used, then we want to emit the names section" (is that correct?)

kripken · 2017-05-23T23:48:06Z

src/wasm/wasm-binary.cpp

+
+  if (!findField("sources", strlen("sources")))
+    throw MapParseException("cannot find the sources field in map");
+  mustReadChar('[');


indentation looks wrong here?

kripken · 2017-05-23T23:48:20Z

src/wasm/wasm-binary.cpp

+    str = std::string(vec.begin(), vec.end());
+  };
+
+  if (!findField("sources", strlen("sources")))


if body of if is on another line, please use {,}

kripken · 2017-05-23T23:48:55Z

src/wasm/wasm-binary.cpp

@@ -1627,6 +1818,15 @@ BinaryConsts::ASTNodes WasmBinaryBuilder::readExpression(Expression*& curr) {
    throw ParseException("Reached function end without seeing End opcode");
  }
  if (debug) std::cerr << "zz recurse into " << ++depth << " at " << pos << std::endl;
+  if (nextDebugLocation.first) {
+      while (nextDebugLocation.first && nextDebugLocation.first <= pos) {


indentation looks larger than the 2 it should be

kripken · 2017-05-23T23:49:11Z

src/wasm/wasm-binary.cpp

@@ -1661,6 +1861,8 @@ BinaryConsts::ASTNodes WasmBinaryBuilder::readExpression(Expression*& curr) {
      throw ParseException("bad node code " + std::to_string(code));
    }
  }
+  if (useDebugLocation && curr)


if identation is too big, and need { }

kripken · 2017-05-23T23:49:58Z

src/wasm/wasm-s-parser.cpp

+  while (debugLocEnd[0] && debugLocEnd[0] != '\n') debugLocEnd++;
+  char* pos = debugLoc;
+  while (pos < debugLocEnd && pos[0] != ':') pos++;
+  if (pos >= debugLocEnd)


also here and later down, need {}

kripken · 2017-05-26T18:01:48Z

auto_update_tests.py

@@ -41,6 +41,10 @@
        print ' '.join(cmd)
        actual = run_command(cmd)
        with open(os.path.join('test', wasm), 'w') as o: o.write(actual)
+        if 'debugInfo' in asm:
+          cmd += ['--source-map', os.path.join('test', wasm + '.map'), '-o', 'a.wasm']


is a.wasm the right place for this? it should be stored where check.py looks for it.

the code above generates proper data for test;a.wasm is just a file we can delete -- we only need source map from this run.

I see, thanks. Sorry for my confusion.

kripken · 2017-05-30T18:24:30Z

Ok, this lgtm. Did anyone else want to take a look?

kripken · 2017-06-01T19:52:51Z

Looks like no concerns, so merging. Thanks!

kripken reviewed May 19, 2017

View reviewed changes

yurydelendik force-pushed the debuginfo branch from 3f94250 to bfbf3b5 Compare May 19, 2017 20:21

kripken reviewed May 19, 2017

View reviewed changes

yurydelendik force-pushed the debuginfo branch from bfbf3b5 to c664179 Compare May 19, 2017 21:19

yurydelendik force-pushed the debuginfo branch from c664179 to 8171be8 Compare May 22, 2017 13:29

yurydelendik force-pushed the debuginfo branch from 01a346a to 94a5faf Compare May 23, 2017 13:23

yurydelendik force-pushed the debuginfo branch from 94a5faf to db10f1a Compare May 23, 2017 14:39

kripken reviewed May 23, 2017

View reviewed changes

yurydelendik force-pushed the debuginfo branch 2 times, most recently from 9facd16 to a2203ec Compare May 23, 2017 22:21

kripken reviewed May 23, 2017

View reviewed changes

yurydelendik force-pushed the debuginfo branch 2 times, most recently from 2dcf466 to 8062530 Compare May 24, 2017 18:03

dschuff and others added 4 commits May 24, 2017 16:42

Exports .txtmap file when .wasm is exported from asm2wasm.

19b2ea5

Extends wasm-as, wasm-dis and s2wasm to consume debug locations.

da4f615

Output JS source mapping file (instead of .txtmap).

9fcdc20

Renames paramenters from binarymap to sourcemap.

f7bd8c9

yurydelendik force-pushed the debuginfo branch from 8062530 to f7bd8c9 Compare May 24, 2017 21:45

kripken reviewed May 26, 2017

View reviewed changes

kripken merged commit 0dc07ea into WebAssembly:master Jun 1, 2017

yurydelendik mentioned this pull request Jun 23, 2017

Enable wasm source maps. emscripten-core/emscripten#5335

Merged

JesseCodeBones mentioned this pull request May 29, 2023

Fix: remove excess optimization for sourcemap #5547

Closed

		@@ -665,22 +668,31 @@ class S2WasmBuilder {

		mustMatch(":");

		Function::DebugLocation debugLocation = {0,0,0};

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

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

Conversation

yurydelendik commented May 19, 2017 • edited Loading

yurydelendik commented May 19, 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

kripken commented May 19, 2017

yurydelendik commented May 19, 2017

kripken commented May 19, 2017

yurydelendik commented May 19, 2017

yurydelendik commented May 19, 2017

kripken commented May 22, 2017

yurydelendik commented May 22, 2017

yurydelendik commented May 23, 2017

Choose a reason for hiding this comment

yurydelendik May 23, 2017 • edited Loading

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

yurydelendik May 23, 2017 • edited Loading

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

kripken commented May 30, 2017

kripken commented Jun 1, 2017

yurydelendik commented May 19, 2017 •

edited

Loading

yurydelendik May 23, 2017 •

edited

Loading

yurydelendik May 23, 2017 •

edited

Loading