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

kripken · 2019-11-21T01:15:19Z

No description provided.

tests/test_other.py

RReverser · 2019-11-21T13:09:51Z

tools/wasm-sourcemap.py

@@ -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'])


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.

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...

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:

sources are specified as absolute URLs (not great for local files, but can work well enough for public libs).

sources are specified as relative URLs to the source map itself.

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).

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...

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

What browser is basically doing:

Extract sourceMappingURL from the WebAssembly module (this one is {--source-map-base}/out.wasm.map).

Load that source map.

If that source map contains sourceRoot, prepend it to each item in sources.

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.

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?

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.

RReverser

Needs to resolve relative path to a source map.

tools/shared.py

kripken · 2019-11-21T22:31:28Z

Those last commits should fix the test failures, at least the ones I ran locally.

I had to change some tests, which I think was correct, but please take a look at those as I'm not sure.

kripken · 2019-11-22T00:47:34Z

Ok, tests look good!

tests/test_core.py

tests/test_other.py

RReverser · 2019-11-22T12:57:47Z

tools/wasm-sourcemap.py

@@ -264,6 +264,14 @@ def build_sourcemap(entries, code_section_offset, prefixes, collect_sources):
      column = 1
    address = entry['address'] + code_section_offset
    file_name = entry['file']
+    # prefer to emit a relative path to the base path, which is where the


I'm not sure if we should do anything special here... since we're talking about edge cases anyway, consider one where a static server that is hosting / directory under, say, http://somedomain/static/.

Then, let's say Wasm is under /dist/temp.wasm on the disk (corresponding to URL: http://somedomain/static/dist/temp.wasm on server), and source is under /src/temp.c (URL: http://somedomain/static/src/temp.c).

If you use relative paths, then source would be encoded as ../src/temp.c and resolve correctly in URL context, but if you keep it as-is, then it would be encoded as /src/temp.c and resolve to http://somedomain/src/temp.c in browser, which is invalid.

TL;DR - let's just do the simple thing and use relative paths unconditionally, which would happen to also cover edge cases more correctly.

That would break the current other.test_wasm_sourcemap test, it gets this as the source file location:

"wasm-src://../../emscripten/tests/other/wasm_sourcemap/no_main.c"

Is that ok?

(if that's not ok, what would the proper output there be, do you think?)

I see, interesting. Where is wasm-src:// added? We probably should skip resolving relative paths altogether when that option is enabled.

It's added in the test, using the --prefix flag to the source map tool (which I don't understand well).

Maybe we can remove that option? If you and @yurydelendik agree that should be fine as I don't see other users in the codebase.

@yurydelendik Doesn't -fdebug-prefix-map in Clang change DWARF paths in-place to the remapped locations? If it does, then what is the role of custom --prefix handling?

what is the role of custom --prefix handling

To do it after the fact of creating a wasm binary. (-fdebug-prefix-map did not work in rustc at the moment of writing wasm-sourcemap.py)

Sure, but now that both compilers support native remapping, it seems that we can just skip it here?

the --base_path logic is a really narrow case of the --prefix. The former assumes that sources folders will have the same directory structure when published and calculates replacement part automatically, the latter provides more flexibility. FWIW they can exist together (by adding else: for prefixes.sources.resolve line). Sorry, I cannot provide more definite "yes, let's remove it", since I accustomed to the --prefix way. :)

Thanks @RReverser , you are right, I had that mixed up! Should be correct now - use the prefixes if provided, otherwise use a proper relative path.

kripken · 2019-11-26T20:33:39Z

Ah, some other tests now break. There is still the question of what to do with "bad" relative paths.

For example, if I am in /foo/ and I compile a file not under that directory, /bar/file.cpp, then if we use a relative path as in this PR, we get ../bar/file.cpp. Is that better than emitting /bar/file.cpp? (Or are both unuseful anyhow?)

RReverser · 2019-11-27T00:34:43Z

Is that better than emitting /bar/file.cpp? (Or are both unuseful anyhow?)

Yes - I described the difference for precisely same case in above comment thread #9882 (comment).

TL;DR 2 - /bar/file.cpp would be still invalid in browser unless your .wasm + .wasm.map is hosted at the root of the website, while ../bar/file.cpp would work in any nested folders too.

UPD: Note that this works in cases where entire / is hosted, but otherwise neither format will be useful. I'd still opt for the format that has more chances to work.

UPD 2: Also probably worth testing what happens with Windows files on different disks.

RReverser · 2019-11-27T15:47:58Z

On a second thought, I suppose these cases are actually rare, so maybe we can just print error in case of sources and output on different root folders and/or Windows disks? Sounds like this would solve our final problems.

kripken · 2019-11-27T18:02:21Z

Thanks @RReverser , I updated the lsan tests to accept those relative paths then.

I agree just showing an error might be another reasonable path here, but I'm not sure how best to explain to users what to do in the error message, seems nontrivial - I'd rather leave that to followup work if we see it's necessary in practice, with people hitting this issue.

RReverser · 2019-11-27T18:46:47Z

@kripken OK, makes sense to me.

sbc100 · 2019-11-29T22:33:27Z

This breaks the windows builder:

https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket.appspot.com/8895598112699504816/+/steps/Emscripten_testsuite__upstream__other_/0/stdout

sbc100 · 2019-11-29T22:34:14Z

I think I'm going to revert this so we can get a green build over the weekend.

…9882)" This reverts commit 5486c4a.

After #9882 we don't always call .resolve, so the path normalization must be pulled into a place that dominates all code paths.

…#9837 (emscripten-core#9882)

…mscripten-core#9925) After emscripten-core#9882 we don't always call .resolve, so the path normalization must be pulled into a place that dominates all code paths.

kripken added 2 commits November 20, 2019 17:13

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

a213913

Merge remote-tracking branch 'origin/incoming' into rel

c813941

kripken mentioned this pull request Nov 21, 2019

Invalid URLs (absolute paths) in source maps in some cases #9837

Closed

RReverser reviewed Nov 21, 2019

View reviewed changes

tests/test_other.py Outdated Show resolved Hide resolved

RReverser reviewed Nov 21, 2019

View reviewed changes

RReverser requested changes Nov 21, 2019

View reviewed changes

kripken added 6 commits November 21, 2019 13:10

Merge remote-tracking branch 'origin/incoming' into rel

6367083

fix

4c2cf4a

test

842caaf

fix test

acef71d

[ci skip]

a9467bd

for now

18ac680

RReverser reviewed Nov 21, 2019

View reviewed changes

tools/shared.py Show resolved Hide resolved

kripken added 3 commits November 21, 2019 14:24

for now

c9a3bd0

fix

bb04bd9

simpler

917b74c

fastcomp fixes

3b15d9e

kripken requested review from dschuff and yurydelendik November 22, 2019 00:48

RReverser reviewed Nov 22, 2019

View reviewed changes

tests/test_core.py Show resolved Hide resolved

RReverser reviewed Nov 22, 2019

View reviewed changes

tests/test_other.py Show resolved Hide resolved

RReverser reviewed Nov 22, 2019

View reviewed changes

tests/test_other.py Show resolved Hide resolved

RReverser reviewed Nov 22, 2019

View reviewed changes

kripken added 2 commits November 26, 2019 11:15

Merge remote-tracking branch 'origin/incoming' into rel

6bdc495

review feedback

8412ca7

kripken added 2 commits November 27, 2019 09:57

Merge remote-tracking branch 'origin/incoming' into rel

2174057

update lsan tests to allow relative paths in stack traces

f2b9909

flake8

450097c

kripken merged commit 5486c4a into incoming Nov 27, 2019

kripken deleted the rel branch November 27, 2019 20:21

sbc100 added a commit that referenced this pull request Nov 29, 2019

Revert "Fix relative paths in wasm backend source maps. Fixes #9837 (#…

6c082e6

…9882)" This reverts commit 5486c4a.

sbc100 mentioned this pull request Nov 29, 2019

Revert "Fix relative paths in wasm backend source maps. Fixes #9837" #9924

Closed

kripken added a commit that referenced this pull request Nov 29, 2019

Fix source maps issues on windows; followup to #9882

c78b2a8

kripken mentioned this pull request Nov 29, 2019

Fix source maps issues on windows; followup to #9882 #9925

Merged

kripken added a commit that referenced this pull request Nov 30, 2019

Fix source maps issues on windows; followup to #9882 (#9925)

afc2573

After #9882 we don't always call .resolve, so the path normalization must be pulled into a place that dominates all code paths.

belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020

Fix relative paths in wasm backend source maps. Fixes emscripten-core…

959529d

…#9837 (emscripten-core#9882)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kripken commented Nov 21, 2019

RReverser Nov 21, 2019

kripken Nov 21, 2019

RReverser Nov 21, 2019

RReverser Nov 21, 2019

RReverser Nov 21, 2019

RReverser Nov 21, 2019

kripken Nov 21, 2019

RReverser Nov 21, 2019

RReverser left a comment

kripken commented Nov 21, 2019

kripken commented Nov 22, 2019

RReverser Nov 22, 2019

kripken Nov 22, 2019

kripken Nov 22, 2019

RReverser Nov 23, 2019

kripken Nov 25, 2019

RReverser Nov 26, 2019

yurydelendik Nov 26, 2019

RReverser Nov 26, 2019

yurydelendik Nov 26, 2019

kripken Nov 26, 2019

kripken commented Nov 26, 2019

RReverser commented Nov 27, 2019 •

edited

Loading

RReverser commented Nov 27, 2019

kripken commented Nov 27, 2019

RReverser commented Nov 27, 2019

sbc100 commented Nov 29, 2019

sbc100 commented Nov 29, 2019

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

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

Conversation

kripken commented Nov 21, 2019

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

RReverser left a comment

Choose a reason for hiding this comment

kripken commented Nov 21, 2019

kripken commented Nov 22, 2019

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 Nov 26, 2019

RReverser commented Nov 27, 2019 • edited Loading

RReverser commented Nov 27, 2019

kripken commented Nov 27, 2019

RReverser commented Nov 27, 2019

sbc100 commented Nov 29, 2019

sbc100 commented Nov 29, 2019

RReverser commented Nov 27, 2019 •

edited

Loading