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.

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

Add sourcemap support to wasm-metadce and wasm-merge #6372

Merged
merged 7 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions src/ir/debug.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ inline void copyDebugInfo(Expression* origin,
Expression* copy,
Function* originFunc,
Function* copyFunc) {
if (originFunc->debugLocations.empty()) {
return; // No debug info to copy
}

struct Lister : public PostWalker<Lister, UnifiedExpressionVisitor<Lister>> {
std::vector<Expression*> list;
void visitExpression(Expression* curr) { list.push_back(curr); }
Expand Down
60 changes: 56 additions & 4 deletions src/ir/module-utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

#include "module-utils.h"
#include "ir/debug.h"
#include "ir/intrinsics.h"
#include "ir/manipulation.h"
#include "ir/properties.h"
Expand All @@ -23,17 +24,46 @@

namespace wasm::ModuleUtils {

// Update the file name indices when moving a set of debug locations from one
// module to another.
static void updateLocationSet(std::set<Function::DebugLocation>& locations,
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment explaining what this function does.

std::vector<Index>& fileIndexMap) {
std::set<Function::DebugLocation> updatedLocations;

for (auto iter : locations) {
iter.fileIndex = fileIndexMap[iter.fileIndex];
updatedLocations.insert(iter);
}
locations.clear();
std::swap(locations, updatedLocations);
}

// Copies a function into a module. If newName is provided it is used as the
// name of the function (otherwise the original name is copied).
Function* copyFunction(Function* func, Module& out, Name newName) {
// name of the function (otherwise the original name is copied). If fileIndexMap
// is specified, it is used to rename source map filename indices when copying
// the function from one module to another one.
Function* copyFunction(Function* func,
Module& out,
Name newName,
std::optional<std::vector<Index>> fileIndexMap) {
auto ret = std::make_unique<Function>();
ret->name = newName.is() ? newName : func->name;
ret->type = func->type;
ret->vars = func->vars;
ret->localNames = func->localNames;
ret->localIndices = func->localIndices;
ret->debugLocations = func->debugLocations;
ret->body = ExpressionManipulator::copy(func->body, out);
debug::copyDebugInfo(func->body, ret->body, func, ret.get());
ret->prologLocation = func->prologLocation;
ret->epilogLocation = func->epilogLocation;
// Update file indices if needed
if (fileIndexMap) {
for (auto& iter : ret->debugLocations) {
iter.second.fileIndex = (*fileIndexMap)[iter.second.fileIndex];
}
updateLocationSet(ret->prologLocation, *fileIndexMap);
updateLocationSet(ret->epilogLocation, *fileIndexMap);
}
ret->module = func->module;
ret->base = func->base;
ret->noFullInline = func->noFullInline;
Expand Down Expand Up @@ -136,8 +166,30 @@ DataSegment* copyDataSegment(const DataSegment* segment, Module& out) {
// Copies named toplevel module items (things of kind ModuleItemKind). See
// copyModule() for something that also copies exports, the start function, etc.
void copyModuleItems(const Module& in, Module& out) {
// If the source module has some debug information, we first compute how
// to map file name indices from this modules to file name indices in
// the target module.
std::optional<std::vector<Index>> fileIndexMap;
if (!in.debugInfoFileNames.empty()) {
std::unordered_map<std::string, Index> debugInfoFileIndices;
for (Index i = 0; i < out.debugInfoFileNames.size(); i++) {
debugInfoFileIndices[out.debugInfoFileNames[i]] = i;
}
fileIndexMap.emplace();
for (Index i = 0; i < in.debugInfoFileNames.size(); i++) {
std::string file = in.debugInfoFileNames[i];
auto iter = debugInfoFileIndices.find(file);
if (iter == debugInfoFileIndices.end()) {
Index index = out.debugInfoFileNames.size();
out.debugInfoFileNames.push_back(file);
debugInfoFileIndices[file] = index;
}
fileIndexMap->push_back(debugInfoFileIndices[file]);
}
}

for (auto& curr : in.functions) {
copyFunction(curr.get(), out);
copyFunction(curr.get(), out, Name(), fileIndexMap);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
for (auto& curr : in.globals) {
copyGlobal(curr.get(), out);
Expand Down
10 changes: 8 additions & 2 deletions src/ir/module-utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,14 @@
namespace wasm::ModuleUtils {

// Copies a function into a module. If newName is provided it is used as the
// name of the function (otherwise the original name is copied).
Function* copyFunction(Function* func, Module& out, Name newName = Name());
// name of the function (otherwise the original name is copied). If fileIndexMap
// is specified, it is used to rename source map filename indices when copying
// the function from one module to another one.
Function*
copyFunction(Function* func,
Module& out,
Name newName = Name(),
std::optional<std::vector<Index>> fileIndexMap = std::nullopt);

Global* copyGlobal(Global* global, Module& out);

Expand Down
4 changes: 1 addition & 3 deletions src/passes/Inlining.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -456,9 +456,7 @@ static Expression* doInlining(Module* module,
}
// Generate and update the inlined contents
auto* contents = ExpressionManipulator::copy(from->body, *module);
if (!from->debugLocations.empty()) {
debug::copyDebugInfo(from->body, contents, from, into);
}
debug::copyDebugInfo(from->body, contents, from, into);
updater.walk(contents);
block->list.push_back(contents);
block->type = retType;
Expand Down
49 changes: 47 additions & 2 deletions src/tools/wasm-merge.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,9 @@ int main(int argc, const char* argv[]) {
std::vector<std::string> inputFileNames;
bool emitBinary = true;
bool debugInfo = false;
std::map<size_t, std::string> inputSourceMapFilenames;
std::string outputSourceMapFilename;
std::string outputSourceMapUrl;

const std::string WasmMergeOption = "wasm-merge options";

Expand All @@ -464,7 +467,11 @@ For example,

will read foo.wasm and bar.wasm, with names 'foo' and 'bar' respectively, so if the second imports from 'foo', we will see that as an import from the first module after the merge. The merged output will be written to merged.wasm.

Note that filenames and modules names are interleaved (which is hopefully less confusing).)");
Note that filenames and modules names are interleaved (which is hopefully less confusing).

Input source maps can be specified by adding an -ism option right after the module name:

wasm-merge foo.wasm foo -ism foo.wasm.map ...)");

options
.add("--output",
Expand All @@ -485,6 +492,37 @@ Note that filenames and modules names are interleaved (which is hopefully less c
inputFileNames.push_back(argument);
}
})
.add("--input-source-map",
"-ism",
"Consume source maps from the specified files",
WasmMergeOption,
Options::Arguments::N,
[&](Options* o, const std::string& argument) {
size_t pos = inputFiles.size();
if (pos == 0 || pos != inputFileNames.size() ||
inputSourceMapFilenames.count(pos - 1)) {
std::cerr << "Option '-ism " << argument
<< "' should be right after the module name\n";
exit(EXIT_FAILURE);
}
inputSourceMapFilenames.insert({pos - 1, argument});
})
.add("--output-source-map",
"-osm",
"Emit source map to the specified file",
WasmMergeOption,
Options::Arguments::One,
[&outputSourceMapFilename](Options* o, const std::string& argument) {
outputSourceMapFilename = argument;
})
.add("--output-source-map-url",
"-osu",
"Emit specified string as source map URL",
WasmMergeOption,
Options::Arguments::One,
[&outputSourceMapUrl](Options* o, const std::string& argument) {
outputSourceMapUrl = argument;
})
.add("--rename-export-conflicts",
"-rec",
"Rename exports to avoid conflicts (rather than error)",
Expand Down Expand Up @@ -529,6 +567,9 @@ Note that filenames and modules names are interleaved (which is hopefully less c
for (Index i = 0; i < inputFiles.size(); i++) {
auto inputFile = inputFiles[i];
auto inputFileName = inputFileNames[i];
auto iter = inputSourceMapFilenames.find(i);
auto inputSourceMapFilename =
(iter == inputSourceMapFilenames.end()) ? "" : iter->second;

if (options.debug) {
std::cerr << "reading input '" << inputFile << "' as '" << inputFileName
Expand All @@ -550,7 +591,7 @@ Note that filenames and modules names are interleaved (which is hopefully less c

ModuleReader reader;
try {
reader.read(inputFile, *currModule);
reader.read(inputFile, *currModule, inputSourceMapFilename);
} catch (ParseException& p) {
p.dump(std::cerr);
Fatal() << "error in parsing wasm input: " << inputFile;
Expand Down Expand Up @@ -606,6 +647,10 @@ Note that filenames and modules names are interleaved (which is hopefully less c
ModuleWriter writer;
writer.setBinary(emitBinary);
writer.setDebugInfo(debugInfo);
if (outputSourceMapFilename.size()) {
writer.setSourceMapFilename(outputSourceMapFilename);
writer.setSourceMapUrl(outputSourceMapUrl);
}
writer.write(merged, options.extra["output"]);
}
}
33 changes: 32 additions & 1 deletion src/tools/wasm-metadce.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,9 @@ int main(int argc, const char* argv[]) {
bool debugInfo = false;
std::string graphFile;
bool dump = false;
std::string inputSourceMapFilename;
std::string outputSourceMapFilename;
std::string outputSourceMapUrl;

const std::string WasmMetaDCEOption = "wasm-opt options";

Expand Down Expand Up @@ -423,6 +426,30 @@ int main(int argc, const char* argv[]) {
o->extra["output"] = argument;
Colors::setEnabled(false);
})
.add("--input-source-map",
"-ism",
"Consume source map from the specified file",
WasmMetaDCEOption,
Options::Arguments::One,
[&inputSourceMapFilename](Options* o, const std::string& argument) {
inputSourceMapFilename = argument;
})
.add("--output-source-map",
"-osm",
"Emit source map to the specified file",
WasmMetaDCEOption,
Options::Arguments::One,
[&outputSourceMapFilename](Options* o, const std::string& argument) {
outputSourceMapFilename = argument;
})
.add("--output-source-map-url",
"-osu",
"Emit specified string as source map URL",
WasmMetaDCEOption,
Options::Arguments::One,
[&outputSourceMapUrl](Options* o, const std::string& argument) {
outputSourceMapUrl = argument;
})
.add("--emit-text",
"-S",
"Emit text instead of binary for the output file",
Expand Down Expand Up @@ -470,7 +497,7 @@ int main(int argc, const char* argv[]) {
ModuleReader reader;
reader.setDWARF(debugInfo);
try {
reader.read(options.extra["infile"], wasm);
reader.read(options.extra["infile"], wasm, inputSourceMapFilename);
} catch (ParseException& p) {
p.dump(std::cerr);
Fatal() << "error in parsing wasm input";
Expand Down Expand Up @@ -578,6 +605,10 @@ int main(int argc, const char* argv[]) {
ModuleWriter writer;
writer.setBinary(emitBinary);
writer.setDebugInfo(debugInfo);
if (outputSourceMapFilename.size()) {
writer.setSourceMapFilename(outputSourceMapFilename);
writer.setSourceMapUrl(outputSourceMapUrl);
}
Copy link
Member

Choose a reason for hiding this comment

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

Please add a test for wasm-metadce too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have also updated the test for wasm-metadce to test the -ism and -osm flags as well.

writer.write(wasm, options.extra["output"]);
}

Expand Down
12 changes: 12 additions & 0 deletions test/lit/help/wasm-merge.test
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
;; CHECK-NEXT:
;; CHECK-NEXT: Note that filenames and modules names are interleaved (which is hopefully less
;; CHECK-NEXT: confusing).
;; CHECK-NEXT:
;; CHECK-NEXT: Input source maps can be specified by adding an -ism option right after the
;; CHECK-NEXT: module name:
;; CHECK-NEXT:
;; CHECK-NEXT: wasm-merge foo.wasm foo -ism foo.wasm.map ...
;; CHECK-NEXT: ================================================================================
;; CHECK-NEXT:
;; CHECK-NEXT:
Expand All @@ -22,6 +27,13 @@
;; CHECK-NEXT:
;; CHECK-NEXT: --output,-o Output file (stdout if not specified)
;; CHECK-NEXT:
;; CHECK-NEXT: --input-source-map,-ism Consume source maps from the specified
;; CHECK-NEXT: file
;; CHECK-NEXT:
;; CHECK-NEXT: --output-source-map,-osm Emit source map to the specified file
;; CHECK-NEXT:
;; CHECK-NEXT: --output-source-map-url,-osu Emit specified string as source map URL
;; CHECK-NEXT:
;; CHECK-NEXT: --rename-export-conflicts,-rec Rename exports to avoid conflicts (rather
;; CHECK-NEXT: than error)
;; CHECK-NEXT:
Expand Down
7 changes: 7 additions & 0 deletions test/lit/help/wasm-metadce.test
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@
;; CHECK-NEXT:
;; CHECK-NEXT: --output,-o Output file (stdout if not specified)
;; CHECK-NEXT:
;; CHECK-NEXT: --input-source-map,-ism Consume source map from the specified
;; CHECK-NEXT: file
;; CHECK-NEXT:
;; CHECK-NEXT: --output-source-map,-osm Emit source map to the specified file
;; CHECK-NEXT:
;; CHECK-NEXT: --output-source-map-url,-osu Emit specified string as source map URL
;; CHECK-NEXT:
;; CHECK-NEXT: --emit-text,-S Emit text instead of binary for the
;; CHECK-NEXT: output file
;; CHECK-NEXT:
Expand Down
57 changes: 57 additions & 0 deletions test/lit/merge/sourcemap.wat
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.

;; RUN: wasm-merge %s first %s.second second -S -o - | filecheck %s --check-prefix=CHECK-TEXT
;; RUN: wasm-as %s -o %t.wasm --source-map %t.map
;; RUN: wasm-as %s.second -o %t.second.wasm --source-map %t.second.map
;; RUN: wasm-merge %t.wasm first --input-source-map %t.map %t.second.wasm second --input-source-map %t.second.map -o %t.merged.wasm --output-source-map %t.merged.map
;; RUN: wasm-dis %t.merged.wasm --source-map %t.merged.map -o - | filecheck %s --check-prefix=CHECK-BIN

;; Test that sourcemap information is preserved

(module
;;@ a:1:1
(func (export "f")
;;@ a:2:1
(nop)
;;@ a:3:1
)
)
;; CHECK-TEXT: (type $0 (func))

;; CHECK-TEXT: (export "f" (func $0))

;; CHECK-TEXT: (export "g" (func $0_1))

;; CHECK-TEXT: ;;@ a:1:1
;; CHECK-TEXT-NEXT: (func $0
;; CHECK-TEXT-NEXT: ;;@ a:2:1
;; CHECK-TEXT-NEXT: (nop)
;; CHECK-TEXT-NEXT: ;;@ a:3:1
;; CHECK-TEXT-NEXT: )

;; CHECK-TEXT: ;;@ b:1:2
;; CHECK-TEXT-NEXT: (func $0_1
;; CHECK-TEXT-NEXT: ;;@ b:2:2
;; CHECK-TEXT-NEXT: (nop)
;; CHECK-TEXT-NEXT: ;;@ b:3:2
;; CHECK-TEXT-NEXT: )

;; CHECK-BIN: (type $0 (func))

;; CHECK-BIN: (export "f" (func $0))

;; CHECK-BIN: (export "g" (func $1))

;; CHECK-BIN: ;;@ a:1:1
;; CHECK-BIN-NEXT: (func $0
;; CHECK-BIN-NEXT: ;;@ a:2:1
;; CHECK-BIN-NEXT: (nop)
;; CHECK-BIN-NEXT: ;;@ a:3:1
;; CHECK-BIN-NEXT: )

;; CHECK-BIN: ;;@ b:1:2
;; CHECK-BIN-NEXT: (func $1
;; CHECK-BIN-NEXT: ;;@ b:2:2
;; CHECK-BIN-NEXT: (nop)
;; CHECK-BIN-NEXT: ;;@ b:3:2
;; CHECK-BIN-NEXT: )
8 changes: 8 additions & 0 deletions test/lit/merge/sourcemap.wat.second
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
(module
;;@ b:1:2
(func (export "g")
;;@ b:2:2
(nop)
;;@ b:3:2
)
)
Loading
Loading