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

Fix relative paths in wasm backend source maps. Fixes #9837 #9882

Merged
merged 17 commits into from
Nov 27, 2019
Merged

Fix relative paths in wasm backend source maps. Fixes #9837 #9882

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a213913
Fix relative paths in wasm backend source maps. Fixes #9837
kripken Nov 21, 2019
c813941
Merge remote-tracking branch 'origin/incoming' into rel
kripken Nov 21, 2019
6367083
Merge remote-tracking branch 'origin/incoming' into rel
kripken Nov 21, 2019
4c2cf4a
fix
kripken Nov 21, 2019
842caaf
test
kripken Nov 21, 2019
acef71d
fix test
kripken Nov 21, 2019
a9467bd
[ci skip]
kripken Nov 21, 2019
18ac680
for now
kripken Nov 21, 2019
c9a3bd0
for now
kripken Nov 21, 2019
bb04bd9
fix
kripken Nov 21, 2019
917b74c
simpler
kripken Nov 21, 2019
3b15d9e
fastcomp fixes
kripken Nov 21, 2019
6bdc495
Merge remote-tracking branch 'origin/incoming' into rel
kripken Nov 26, 2019
8412ca7
review feedback
kripken Nov 26, 2019
2174057
Merge remote-tracking branch 'origin/incoming' into rel
kripken Nov 27, 2019
f2b9909
update lsan tests to allow relative paths in stack traces
kripken Nov 27, 2019
450097c
flake8
kripken Nov 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions tests/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8916,6 +8916,21 @@ def test_wasm_sourcemap_dead(self):
# has only two entries
self.assertRegexpMatches(output, r'"mappings":\s*"[A-Za-z0-9+/]+,[A-Za-z0-9+/]+"')

def test_wasm_sourcemap_relative_paths(self):
filename = 'a.cpp'
shutil.copyfile(path_from_root('tests', 'hello_123.c'), 'a.cpp')
filenames = [
filename,
os.path.abspath(filename)
]
if not WINDOWS:
kripken marked this conversation as resolved.
Show resolved Hide resolved
filenames.append('./' + filename)
for curr in filenames:
print(curr)
run_process([PYTHON, EMCC, curr, '-g4'])
with open('a.out.wasm.map', 'r') as f:
self.assertIn('"a.cpp"', str(f.read()))

def test_wasm_producers_section(self):
# no producers section by default
run_process([PYTHON, EMCC, path_from_root('tests', 'hello_world.c')])
Expand Down
2 changes: 1 addition & 1 deletion tools/wasm-sourcemap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ def build_sourcemap(entries, code_section_offset, prefixes, collect_sources):
if column == 0:
column = 1
address = entry['address'] + code_section_offset
file_name = entry['file']
file_name = os.path.relpath(entry['file'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this is correct: it uses a path relative to the current directory, but browsers / spec expect sources to be relative to the source map file.

For example, consider case like following:

$ emcc ./temp.c -g4 -o dist/temp.wasm

Currently this produces a source map dist/temp.wasm.map with following contents:

{"version":3,"sources":["./temp.c"],...

But this is incorrect, because temp.c is actually in a folder up and needs to be referenced as ../temp.c.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case wouldn't the user need to set up --source-map-base to fix that? But I've never really understood any of this stuff, hopefully you do...

Copy link
Collaborator

Choose a reason for hiding this comment

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

So --source-map-base helps browser to find a source map from WebAssembly module.

But once browser found that source map, it now needs to find source files. There are three potential ways for it to do that:

  1. sources are specified as absolute URLs (not great for local files, but can work well enough for public libs).
  2. sources are specified as relative URLs to the source map itself.
  3. Source map contains sourceRoot, and then all sources are relative to that instead (could be used in our case as an alternative, but it's a bit more complicated IMO).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, maybe you're right, and in this case --source-map-base might be enough, at least if .wasm.map is always emitted alongside .wasm. Hmm...

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, wait, scratch that, because relative URL still has to be relative to the source map itself. So the comment before still stands :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

What browser is basically doing:

  1. Extract sourceMappingURL from the WebAssembly module (this one is {--source-map-base}/out.wasm.map).
  2. Load that source map.
  3. If that source map contains sourceRoot, prepend it to each item in sources.
  4. Resolve each item in sources relative to sourceMappingURL.

So we need each item in sources to be relative to out.wasm.map, no matter where it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I think I see a little better. I'm still confused about how that sourceMappingURL option is used in two places for seemingly different purposes, though?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In both places it's used as "URL of the .wasm.map". First, to find the map, and later to resolve other files it references, but semantic meaning is the same.

It's similar to how most module systems work, too: if A imports B, and B imports C, then first B is found from A, and then reference to C is resolve relatively to B.

source_name = prefixes.sources.resolve(file_name)
if source_name not in sources_map:
source_id = len(sources)
Expand Down