Skip to content

Conversation

@kripken
Copy link
Member

@kripken kripken commented Oct 22, 2024

We already generated (throw ..) instructions in wasm, but it makes sense to model
throws from outside as well, as they cross the module boundary. This adds a new fuzzer
import to the generated modules, "throw", that just does a throw from JS etc.

Diff without whitespace is smaller.

@kripken kripken requested review from aheejin and tlively October 22, 2024 18:39
…ng-support, and logging funcs must start with 'log-'
// Throw something. We use a (hopefully) private name here.
auto payload = std::make_shared<ExnData>("__private", Literals{});
throwException(WasmException{Literal(payload)});
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an else { WASM_UNREACHABLE(... after this to catch problems earlier if we add a new import that does not follow any of these schemes?

// something not exported if out of bounds. First we must also export
// tags sometimes.
throwImportName = Names::getValidFunctionName(wasm, "throw");
auto* func = new Function;
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a std::unique_ptr here.

@kripken
Copy link
Member Author

kripken commented Oct 22, 2024

Thanks, feedback should be addressed.

I also realized we can test this directly, and added that.

@kripken
Copy link
Member Author

kripken commented Oct 22, 2024

New testcase found an issue with hardcoded logging import names. They need to be flexible, like the throwing import one already was. Fixed.

@kripken
Copy link
Member Author

kripken commented Oct 22, 2024

Also we must compute those names before we use them, a slight reordering in how we modify functions.

@aheejin
Copy link
Member

aheejin commented Oct 22, 2024

Why would the logging function name changes be related to the throwing capability? And in which way are we changing the logging names (other than storing them in a map)?

@kripken
Copy link
Member Author

kripken commented Oct 22, 2024

@aheejin Before this PR, execution-results.h treated all imports from the "fuzzing-support" module as logging functions. After this PR, it only treats those with a name starting with "log*" that way.

By itself that shouldn't change much, but the fuzzer started to error on these tests because it was seeing initial content that already used a name like (func $log-i32 .., so when the fuzzer tried to add another such function, it failed. Instead the fuzzer now uses getValidFunctionName to make up the new names, saves it in the map, and calls the functions using those names.

@kripken
Copy link
Member Author

kripken commented Oct 22, 2024

And I think the fuzzer started to fail because it ran into those files a lot more because they have been recently updated in this PR. The problem existed before AFAICT.

@kripken kripken merged commit dcc70bb into WebAssembly:main Oct 23, 2024
13 checks passed
@kripken kripken deleted the fuzz.throw.js branch October 23, 2024 17:17
@kripken kripken mentioned this pull request Oct 30, 2024
kripken added a commit that referenced this pull request Oct 31, 2024
Continues the work from #7027 which added throwing from JS, this adds
table get/set operations from JS, to further increase our coverage of
Wasm/JS interactions (the table can be used from both sides).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants