Skip to content

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

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

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 12 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
12 changes: 9 additions & 3 deletions tests/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7179,6 +7179,12 @@ def encode_utf8(data):
else:
return data

def source_map_file_loc(name):
if shared.Settings.WASM_BACKEND:
return name
# in fastcomp, we have the absolute path, which is not good
return os.path.abspath(name)

data = json.load(open(map_filename))
if str is bytes:
# Python 2 compatibility
Expand All @@ -7188,7 +7194,7 @@ def encode_utf8(data):
# the output file.
self.assertPathsIdentical(map_referent, data['file'])
assert len(data['sources']) == 1, data['sources']
self.assertPathsIdentical(os.path.abspath('src.cpp'), data['sources'][0])
self.assertPathsIdentical(source_map_file_loc('src.cpp'), data['sources'][0])
if hasattr(data, 'sourcesContent'):
# the sourcesContent attribute is optional, but if it is present it
# needs to containt valid source text.
Expand All @@ -7201,7 +7207,7 @@ def encode_utf8(data):
mappings = encode_utf8(mappings)
seen_lines = set()
for m in mappings:
self.assertPathsIdentical(os.path.abspath('src.cpp'), m['source'])
self.assertPathsIdentical(source_map_file_loc('src.cpp'), m['source'])
seen_lines.add(m['originalLine'])
# ensure that all the 'meaningful' lines in the original code get mapped
# when optimizing, the binaryen optimizer may remove some of them (by inlining, etc.)
Expand Down Expand Up @@ -8241,7 +8247,7 @@ def test_ubsan_full_static_cast(self, args):
'g4': ('-g4', [
"src.cpp:3:12: runtime error: reference binding to null pointer of type 'int'",
'in main ',
'/src.cpp:3:8'
'src.cpp:3:8'
]),
})
@no_fastcomp('ubsan not supported on fastcomp')
Expand Down
28 changes: 26 additions & 2 deletions tests/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8895,7 +8895,8 @@ def test_wasm_sourcemap(self):
'--dwarfdump-output',
path_from_root('tests', 'other', 'wasm_sourcemap', 'foo.wasm.dump'),
'-o', 'a.out.wasm.map',
path_from_root('tests', 'other', 'wasm_sourcemap', 'foo.wasm')]
path_from_root('tests', 'other', 'wasm_sourcemap', 'foo.wasm'),
'--basepath=' + os.getcwd()]
run_process(wasm_map_cmd)
output = open('a.out.wasm.map').read()
# has "sources" entry with file (includes also `--prefix =wasm-src:///` replacement)
Expand All @@ -8910,12 +8911,35 @@ def test_wasm_sourcemap_dead(self):
'--dwarfdump-output',
path_from_root('tests', 'other', 'wasm_sourcemap_dead', 't.wasm.dump'),
'-o', 'a.out.wasm.map',
path_from_root('tests', 'other', 'wasm_sourcemap_dead', 't.wasm')]
path_from_root('tests', 'other', 'wasm_sourcemap_dead', 't.wasm'),
'--basepath=' + os.getcwd()]
run_process(wasm_map_cmd, stdout=PIPE, stderr=PIPE)
output = open('a.out.wasm.map').read()
# has only two entries
self.assertRegexpMatches(output, r'"mappings":\s*"[A-Za-z0-9+/]+,[A-Za-z0-9+/]+"')

@no_fastcomp()
def test_wasm_sourcemap_relative_paths(self):
def test(infile, source_map_added_dir=''):
expected_source_map_path = os.path.join(source_map_added_dir, 'a.cpp')
print(infile, expected_source_map_path)
shutil.copyfile(path_from_root('tests', 'hello_123.c'), infile)
infiles = [
infile,
os.path.abspath(infile),
'./' + infile
]
for curr in infiles:
print(' ', curr)
run_process([PYTHON, EMCC, curr, '-g4'])
with open('a.out.wasm.map', 'r') as f:
self.assertIn('"%s"' % expected_source_map_path, str(f.read()))

test('a.cpp')

os.mkdir('inner')
test(os.path.join('inner', 'a.cpp'), 'inner')

def test_wasm_producers_section(self):
# no producers section by default
run_process([PYTHON, EMCC, path_from_root('tests', 'hello_world.c')])
Expand Down
6 changes: 5 additions & 1 deletion tools/shared.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2871,10 +2871,14 @@ def path_to_system_js_libraries(library_name):

@staticmethod
def emit_wasm_source_map(wasm_file, map_file):
# source file paths must be relative to the location of the map (which is
# emitted alongside the wasm)
base_path = os.path.dirname(os.path.abspath(Settings.WASM_BINARY_FILE))
sourcemap_cmd = [PYTHON, path_from_root('tools', 'wasm-sourcemap.py'),
wasm_file,
'--dwarfdump=' + LLVM_DWARFDUMP,
'-o', map_file]
'-o', map_file,
'--basepath=' + base_path]
check_call(sourcemap_cmd)

@staticmethod
Expand Down
14 changes: 11 additions & 3 deletions tools/wasm-sourcemap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ def parse_args():
parser.add_argument('-u', '--source-map-url', nargs='?', help='specifies sourceMappingURL section contest')
parser.add_argument('--dwarfdump', help="path to llvm-dwarfdump executable")
parser.add_argument('--dwarfdump-output', nargs='?', help=argparse.SUPPRESS)
parser.add_argument('--basepath', help='base path for source files, which will be relative to this')
return parser.parse_args()


Expand Down Expand Up @@ -243,11 +244,10 @@ def read_dwarf_entries(wasm, options):
return sorted(entries, key=lambda entry: entry['address'])


def build_sourcemap(entries, code_section_offset, prefixes, collect_sources):
def build_sourcemap(entries, code_section_offset, prefixes, collect_sources, base_path):
sources = []
sources_content = [] if collect_sources else None
mappings = []

sources_map = {}
last_address = 0
last_source_id = 0
Expand All @@ -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
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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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 @RReverser , you are right, I had that mixed up! Should be correct now - use the prefixes if provided, otherwise use a proper relative path.

# source map file itself is, so the browser can find it. however, if they
# have no shared prefix (like one is /a/b and the other /c/d) then we
# would not end up with anything more useful (like ../../c/d), so avoid
# that (a shared prefix of size 1, like '/', is useless)
abs_file_name = os.path.abspath(file_name)
if len(os.path.commonprefix([abs_file_name, base_path])) > 1:
file_name = os.path.relpath(abs_file_name, base_path)
source_name = prefixes.sources.resolve(file_name)
if source_name not in sources_map:
source_id = len(sources)
Expand Down Expand Up @@ -311,7 +319,7 @@ def main():
prefixes = SourceMapPrefixes(sources=Prefixes(options.prefix), load=Prefixes(options.load_prefix))

logger.debug('Saving to %s' % options.output)
map = build_sourcemap(entries, code_section_offset, prefixes, options.sources)
map = build_sourcemap(entries, code_section_offset, prefixes, options.sources, options.basepath)
with open(options.output, 'w') as outfile:
json.dump(map, outfile, separators=(',', ':'))

Expand Down