bytecodealliance / wasm-micro-runtime

WebAssembly Micro Runtime (WAMR)
Apache License 2.0
4.94k stars 623 forks source link

EH: tag matching seems broken #3109

Open yamt opened 9 months ago

yamt commented 9 months ago

the current logic decides exception matching (ie. which catch clause to use) by comparing tag indexes. it seems wrong because, if i read the spec correctly, it's allowed for a module to import the same tag multiple times.

ermler commented 2 months ago

@yamt a good question. we looked at "try-catch.wast" that was present when implementing. The files shows a test, where a tag is imported from a module and the catch-block aims at tagindex after importing.

See, how "test.throw" in external module throws "test.e0" https://github.com/WebAssembly/exception-handling/blob/cfe863826484d7d8a630324f1839f7ed3afaffc9/test/core/try_catch.wast#L5

Module imports "test.e0" as "imported-e0" https://github.com/WebAssembly/exception-handling/blob/cfe863826484d7d8a630324f1839f7ed3afaffc9/test/core/try_catch.wast#L11

and the testcase "catch-imported" calls "test.throw" and catches the "test.e0" exception as tagindex "imported-e0": https://github.com/WebAssembly/exception-handling/blob/cfe863826484d7d8a630324f1839f7ed3afaffc9/test/core/try_catch.wast#L136

we did our implementation to behave like this and wrote wamr code that looks up and replaces tagindex after returning from the external call. https://github.com/bytecodealliance/wasm-micro-runtime/blob/e8c2952bf959f6df81972fc469a4357f04ee629a/core/iwasm/interpreter/wasm_interp_classic.c#L6488

but anyhow, it is a good question, which behavior is expected, if the same tag is imported twice and so has multiple catchblocks in the importing module....

yamt commented 2 months ago

i submitted a test case. https://github.com/WebAssembly/exception-handling/pull/326 (it's for the latest version of the proposal though.)

ermler commented 2 months ago

i submitted a test case. WebAssembly/exception-handling#326 (it's for the latest version of the proposal though.)

thanks for clarification, that test case explains the idea.