Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable wasm source maps. #5335

Merged
merged 1 commit into from
Jun 26, 2017
Merged

Enable wasm source maps. #5335

merged 1 commit into from
Jun 26, 2017

Conversation

yurydelendik
Copy link
Collaborator

@yurydelendik yurydelendik commented Jun 23, 2017 •
edited
Loading

Creates source map file for ".wasm" during compilation with -g4. Also adds --source-map-base option to provide a complete solution -- currently the .wasm file must contain sourceMappingURL section to make it work (per WebAssembly/design#1051).

The patch requires binaryen with patch WebAssembly/binaryen#1017

The example output can be found at https://github.com/yurydelendik/wasm-source-map-emscripten/

@yurydelendik
Copy link
Collaborator Author

/cc @dschuff

kripken
kripken requested changes Jun 23, 2017
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks nice, just one minor comment above, and also, please update the binaryen tag to 34 so that it uses a version with source maps support (I pushed that tag to binaryen now, so you just need to edit tools/ports/binaryen.py).

emcc.py Outdated
@@ -1768,7 +1769,7 @@ def get_eliminate():
if shared.Settings.CYBERDWARF:
execute([shared.PYTHON, shared.path_from_root('tools', 'emdebug_cd_merger.py'), target + '.cd', target+'.symbols'])

if options.debug_level >= 4:
if options.debug_level >= 4 and not shared.Settings.BINARYEN:
emit_source_maps(target, optimizer.js_transform_tempfiles)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this method is just for js and not wasm, how about renaming it to emit_js_source_maps?

@yurydelendik
Copy link
Collaborator Author

please update the binaryen tag to 34 so that it uses a version with source maps support

Fixed

@kripken
Copy link
Member

kripken commented Jun 26, 2017

Thanks, looks good. Although I notice now that this doesn't have a test. Let's merge this in for now, but can you please add one later?

@kripken kripken merged commit e99b507 into emscripten-core:incoming Jun 26, 2017
@kripken
Copy link
Member

kripken commented Jul 11, 2017

@yurydelendik this broke binaryen2.test_source_map. perhaps the test needs to be updated for the new format somehow?

@yurydelendik
Copy link
Collaborator Author

the test needs to be updated for the new format somehow?

Yes, the test_source_map test needs to be updated for binaryen* tests to provide right map file name. Also, source-map package is a little bit outdated, e.g. it requires 'file' attribute which is optional. (I will submit patch after I'll done testing it)

@yurydelendik yurydelendik mentioned this pull request Jul 12, 2017
Merged
@yurydelendik yurydelendik deleted the sourcemaps branch July 18, 2017 16:20
buu700 pushed a commit to buu700/emscripten that referenced this pull request Jul 20, 2017
@yurydelendik @buu700
Enable wasm source maps. (emscripten-core#5335)
2adabcd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kripken kripken kripken requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yurydelendik @kripken