-
Notifications
You must be signed in to change notification settings - Fork 747
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
Fuzzer: Add a pass to prune illegal imports and exports for JS #6312
Changes from all commits
f40ad6d
90a8d09
ddb4b98
3b3e5c2
55eec21
29b34db
85aae14
3a5d518
4cc4411
6f7446b
7c1c117
cea8107
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,12 @@ | |
// across modules, we still want to legalize dynCalls so JS can call into the | ||
// tables even to a signature that is not legal. | ||
// | ||
// Another variation also "prunes" imports and exports that we cannot yet | ||
// legalize, like exports and imports with SIMD or multivalue. Until we write | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would we need to legalize or prune out multivalue? We should only be testing on JS engines that fully support it, right? |
||
// the logic to legalize them, removing those imports/exports still allows us to | ||
// fuzz all the legal imports/exports. (Note that multivalue is supported in | ||
// exports in newer VMs - node 16+ - so that part is only needed for older VMs.) | ||
// | ||
|
||
#include "asmjs/shared-constants.h" | ||
#include "ir/element-utils.h" | ||
|
@@ -43,6 +49,8 @@ | |
|
||
namespace wasm { | ||
|
||
namespace { | ||
|
||
// These are aliases for getTempRet0/setTempRet0 which emscripten defines in | ||
// compiler-rt and exports under these names. | ||
static Name GET_TEMP_RET_EXPORT("__get_temp_ret"); | ||
|
@@ -358,10 +366,88 @@ struct LegalizeJSInterface : public Pass { | |
} | ||
}; | ||
|
||
struct LegalizeAndPruneJSInterface : public LegalizeJSInterface { | ||
// Legalize fully (true) and add pruning on top. | ||
LegalizeAndPruneJSInterface() : LegalizeJSInterface(true) {} | ||
|
||
void run(Module* module) override { | ||
LegalizeJSInterface::run(module); | ||
|
||
prune(module); | ||
} | ||
|
||
void prune(Module* module) { | ||
// For each function name, the exported id it is exported with. For | ||
// example, | ||
// | ||
// (func $foo (export "bar") | ||
// | ||
// Would have exportedFunctions["foo"] = "bar"; | ||
std::unordered_map<Name, Name> exportedFunctions; | ||
for (auto& exp : module->exports) { | ||
if (exp->kind == ExternalKind::Function) { | ||
exportedFunctions[exp->value] = exp->name; | ||
} | ||
} | ||
|
||
for (auto& func : module->functions) { | ||
// If the function is neither exported nor imported, no problem. | ||
auto imported = func->imported(); | ||
auto exported = exportedFunctions.count(func->name); | ||
if (!imported && !exported) { | ||
continue; | ||
} | ||
|
||
// The params are allowed to be multivalue, but not the results. Otherwise | ||
// look for SIMD. | ||
auto sig = func->type.getSignature(); | ||
auto illegal = isIllegal(sig.results); | ||
illegal = | ||
illegal || std::any_of(sig.params.begin(), | ||
sig.params.end(), | ||
[&](const Type& t) { return isIllegal(t); }); | ||
if (!illegal) { | ||
continue; | ||
} | ||
|
||
// Prune an import by implementing it in a trivial manner. | ||
if (imported) { | ||
func->module = func->base = Name(); | ||
|
||
Builder builder(*module); | ||
if (sig.results == Type::none) { | ||
func->body = builder.makeNop(); | ||
} else { | ||
func->body = | ||
builder.makeConstantExpression(Literal::makeZeros(sig.results)); | ||
Comment on lines
+421
to
+422
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if the result type is not defaultable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then we'd be in trouble, but we already avoid this situation in the fuzzer because it is a problem in general: We can't send a non-null reference from JS. So the fuzzer fixes that up. |
||
} | ||
} | ||
|
||
// Prune an export by just removing it. | ||
if (exported) { | ||
module->removeExport(exportedFunctions[func->name]); | ||
} | ||
} | ||
|
||
// TODO: globals etc. | ||
} | ||
|
||
bool isIllegal(Type type) { | ||
auto features = type.getFeatures(); | ||
return features.hasSIMD() || features.hasMultivalue(); | ||
} | ||
}; | ||
|
||
} // anonymous namespace | ||
|
||
Pass* createLegalizeJSInterfacePass() { return new LegalizeJSInterface(true); } | ||
|
||
Pass* createLegalizeJSInterfaceMinimallyPass() { | ||
return new LegalizeJSInterface(false); | ||
} | ||
|
||
Pass* createLegalizeAndPruneJSInterfacePass() { | ||
return new LegalizeAndPruneJSInterface(); | ||
} | ||
|
||
} // namespace wasm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then what does it print for v128?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will trap atm if it is called. But this is forward-looking in that if the JS-wasm spec some day allows this without trapping - maybe it becomes a JS array of 4 elements? - then it would print that JS value.