Add sourcemap support to wasm-metadce and wasm-merge#6372
Add sourcemap support to wasm-metadce and wasm-merge#6372kripken merged 7 commits intoWebAssembly:mainfrom
Conversation
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Thanks, this is great!
Overall this looks good aside from some comments and suggestions.
src/ir/module-utils.cpp
Outdated
| namespace wasm::ModuleUtils { | ||
|
|
||
| static void updateLocationSet(std::set<Function::DebugLocation>& locations, | ||
| std::vector<Index>* indexMap) { |
There was a problem hiding this comment.
This would be better I think:
| std::vector<Index>* indexMap) { | |
| std::vector<Index>& indexMap) { |
|
|
||
| namespace wasm::ModuleUtils { | ||
|
|
||
| static void updateLocationSet(std::set<Function::DebugLocation>& locations, |
There was a problem hiding this comment.
Please add a comment explaining what this function does.
src/ir/module-utils.cpp
Outdated
| Function* copyFunction(Function* func, | ||
| Module& out, | ||
| Name newName, | ||
| std::vector<Index>* indexMap) { |
There was a problem hiding this comment.
We prefer this pattern:
| std::vector<Index>* indexMap) { | |
| std::optional<std::vector<Index>> indexMap = std::nullopt) { |
src/ir/module-utils.cpp
Outdated
| Function* copyFunction(Function* func, | ||
| Module& out, | ||
| Name newName, | ||
| std::vector<Index>* indexMap) { |
There was a problem hiding this comment.
Perhaps this could be fileIndexMap to clarify what it refers to?
src/tools/wasm-merge.cpp
Outdated
| // Copy the regular module items (functions, globals) etc. which we | ||
| // have proper names for, and can just copy. | ||
| ModuleUtils::copyModuleItems(input, merged); | ||
| ModuleUtils::copyModuleItems(input, merged, &indexMap); |
There was a problem hiding this comment.
I could be wrong, but the issue of mapping filename indices is potentially present in all calls of copyFunction, unless the entire module is cloned. That is, we can call copyFunction to copy a function from an arbitrary module to another. Given that, perhaps the above logic should be in module-utils so that all users use it? We may not have another user atm in the codebase, but doing it that way would avoid needing to refactor and maybe missing a bug if we do in the future.
| if (outputSourceMapFilename.size()) { | ||
| writer.setSourceMapFilename(outputSourceMapFilename); | ||
| writer.setSourceMapUrl(outputSourceMapUrl); | ||
| } |
There was a problem hiding this comment.
Please add a test for wasm-metadce too.
There was a problem hiding this comment.
I have also updated the test for wasm-metadce to test the -ism and -osm flags as well.
vouillon
changed the title
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Great! Just one last comment.
|
|
||
| for (auto& curr : in.functions) { | ||
| copyFunction(curr.get(), out); | ||
| copyFunction(curr.get(), out, Name(), fileIndexMap); |
There was a problem hiding this comment.
For efficiency, how about only passing a non-nullopt fileIndexMap if !in.debugInfoFileNames.empty() - ? That is, when the input has no debug info, we can avoid this work.
(fileIndexMap can be a std::optional that is only set if we have work to do, so when we don't it'll be nullopt.)
There was a problem hiding this comment.
I have also added a test at the beginning of debug::copyDebugInfo to just return if there is nothing to copy. This seems more foolproof than putting the test outside of the function...
Implement mapping between source and wasm locations. To work, this requires a version of Binaryen compiled with Jérôme's patch WebAssembly/binaryen#6372. Single-stepping can jump around in slightly surprising ways in the OCaml code, due to the different order of operations in wasm. This could be improved by modifying Binaryen to support “no location” annotations. Another future improvement can be to support mapping Wasm identifiers to OCaml ones.
Implement mapping between source and wasm locations. To work, this requires a version of Binaryen compiled with Jérôme's patch WebAssembly/binaryen#6372. Single-stepping can jump around in slightly surprising ways in the OCaml code, due to the different order of operations in wasm. This could be improved by modifying Binaryen to support “no location” annotations. Another future improvement can be to support mapping Wasm identifiers to OCaml ones.
Implement mapping between source and wasm locations. To work, this requires a version of Binaryen compiled with Jérôme's patch WebAssembly/binaryen#6372. Single-stepping can jump around in slightly surprising ways in the OCaml code, due to the different order of operations in wasm. This could be improved by modifying Binaryen to support “no location” annotations. Another future improvement can be to support mapping Wasm identifiers to OCaml ones. Co-authored-by: Jérôme Vouillon <jerome.vouillon@gmail.com>
Implement mapping between source and wasm locations. To work, this requires a version of Binaryen compiled with Jérôme's patch WebAssembly/binaryen#6372. Single-stepping can jump around in slightly surprising ways in the OCaml code, due to the different order of operations in wasm. This could be improved by modifying Binaryen to support “no location” annotations. Another future improvement can be to support mapping Wasm identifiers to OCaml ones. Co-authored-by: Jérôme Vouillon <jerome.vouillon@gmail.com>
Implement mapping between source and wasm locations. To work, this requires a version of Binaryen compiled with Jérôme's patch WebAssembly/binaryen#6372. Single-stepping can jump around in slightly surprising ways in the OCaml code, due to the different order of operations in wasm. This could be improved by modifying Binaryen to support “no location” annotations. Another future improvement can be to support mapping Wasm identifiers to OCaml ones. Co-authored-by: Jérôme Vouillon <jerome.vouillon@gmail.com>
2 participants
This adds appropriate command line arguments.
In the case of wasm-merge, a bit of work was also needed to copy the debug information when copying functions.