From 9affab2262766b10ce0a2480677a5ac6c279faf5 Mon Sep 17 00:00:00 2001 From: Noah Jelich <12912633+njelich@users.noreply.github.com> Date: Mon, 25 Sep 2023 16:39:59 +0200 Subject: [PATCH 1/2] Broken Links Housekeeping (#1214) * Update README.md * Update README.md * Create LICENSE * Delete crates/wasm-compose/LICENSE * Update README.md * Removed dead rust logo url --- crates/wasm-compose/README.md | 2 +- crates/wasm-mutate/README.md | 2 +- crates/wasm-smith/README.md | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/wasm-compose/README.md b/crates/wasm-compose/README.md index 6ab8bc7b1a..644b19329a 100644 --- a/crates/wasm-compose/README.md +++ b/crates/wasm-compose/README.md @@ -75,7 +75,7 @@ of composing WebAssembly components together. ## License This project is licensed under the Apache 2.0 license with the LLVM exception. -See [LICENSE](LICENSE) for more details. +See [LICENSE](../../LICENSE) for more details. ### Contribution diff --git a/crates/wasm-mutate/README.md b/crates/wasm-mutate/README.md index 3eb29d0bdb..fd7e099cf7 100644 --- a/crates/wasm-mutate/README.md +++ b/crates/wasm-mutate/README.md @@ -138,7 +138,7 @@ wasm-tools mutate original.wasm --seed 0 -o out.wasm --preserve-semantics # License This project is licensed under the Apache 2.0 license with the LLVM exception. -See [LICENSE](LICENSE) for more details. +See [LICENSE](../../LICENSE) for more details. ### Contribution diff --git a/crates/wasm-smith/README.md b/crates/wasm-smith/README.md index a4f2603da9..489918aca4 100644 --- a/crates/wasm-smith/README.md +++ b/crates/wasm-smith/README.md @@ -5,7 +5,6 @@ [![](https://docs.rs/wasm-smith/badge.svg)](https://docs.rs/wasm-smith/) [![](https://img.shields.io/crates/v/wasm-smith.svg)](https://crates.io/crates/wasm-smith) [![](https://img.shields.io/crates/d/wasm-smith.svg)](https://crates.io/crates/wasm-smith) -![Rust](https://github.com/fitzgen/wasm-smith/workflows/Rust/badge.svg) * [Features](#features) * [Usage](#usage) @@ -76,7 +75,7 @@ $ cargo fuzz run my_wasm_smith_fuzz_target ``` > **Note:** Also check out [the `validate` fuzz -> target](https://github.com/bytecodealliance/wasm-tools/blob/main/fuzz/fuzz_targets/validate.rs) +> target](https://github.com/bytecodealliance/wasm-tools/blob/main/fuzz/src/validate.rs) > defined in this repository. Using the `wasmparser` crate, it checks that every > module generated by `wasm-smith` validates successfully. From 585a0bdd8f49fc05d076effaa96e63d97f420578 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Mon, 25 Sep 2023 11:32:04 -0500 Subject: [PATCH 2/2] Clean up parsing `(if ...)` (#1215) * Clean up parsing `(if ...)` Much of this code dates back to when this crate was trying to parse all of WABT's tests as well as the upstream test suite. WABT at the time had syntactical forms that weren't technically supposed to be valid given the upstream test suite, so at the time more was accepted than should be. This commit fixes how `(if ...)` is parsed to require `(then ..)` and `(else ..)` wrappers. Previously these were mistakenly only optional. Additionally this fixes #1213 by allowing multiple condition-related folded expressions before `(then ..)`. This updates a few of the golden outputs for the spec test suites, notably those that use `(if (foo) (then) (else))` where previously `wast` would erroneously not emit an `else` instruction but now it does. Closes #1213 * Fix some test syntax --- crates/wast/src/core/expr.rs | 102 +++++++----------- .../adapt-old.wat | 2 +- .../adapt-old.wat | 2 +- .../adapt-old.wat | 2 +- tests/local/fuzz1.wat | 24 ++--- tests/local/if-else-parsing.wast | 42 ++++++++ .../local/if-else-parsing.wast/0.print | 29 +++++ tests/snapshots/testsuite/if.wast/0.print | 2 + .../function-references/if.wast/0.print | 2 + .../testsuite/proposals/gc/if.wast/0.print | 2 + 10 files changed, 130 insertions(+), 79 deletions(-) create mode 100644 tests/local/if-else-parsing.wast create mode 100644 tests/snapshots/local/if-else-parsing.wast/0.print diff --git a/crates/wast/src/core/expr.rs b/crates/wast/src/core/expr.rs index 3fd511a15d..f89624e29b 100644 --- a/crates/wast/src/core/expr.rs +++ b/crates/wast/src/core/expr.rs @@ -98,18 +98,18 @@ enum Level<'a> { TryArm, } -/// Possible states of "what should be parsed next?" in an `if` expression. +/// Possible states of "what is currently being parsed?" in an `if` expression. enum If<'a> { - /// Only the `if` has been parsed, next thing to parse is the clause, if - /// any, of the `if` instruction. + /// Only the `if` instructoin has been parsed, next thing to parse is the + /// clause, if any, of the `if` instruction. + /// + /// This parse ends when `(then ...)` is encountered. Clause(Instruction<'a>), - /// Next thing to parse is the `then` block - Then(Instruction<'a>), - /// Next thing to parse is the `else` block + /// Currently parsing the `then` block, and afterwards a closing paren is + /// required or an `(else ...)` expression. + Then, + /// Parsing the `else` expression, nothing can come after. Else, - /// This `if` statement has finished parsing and if anything remains it's a - /// syntax error. - End, } /// Possible state of "what should be parsed next?" in a `try` expression. @@ -216,9 +216,6 @@ impl<'a> ExpressionParser<'a> { // items in the `if` statement. Otherwise we're just careful // to terminate with an `end` instruction. Level::If(If::Clause(_)) => { - return Err(parser.error("previous `if` had no clause")); - } - Level::If(If::Then(_)) => { return Err(parser.error("previous `if` had no `then`")); } Level::If(_) => { @@ -281,23 +278,16 @@ impl<'a> ExpressionParser<'a> { }) } - /// Handles all parsing of an `if` statement. + /// State transitions with parsing an `if` statement. /// /// The syntactical form of an `if` stament looks like: /// /// ```wat - /// (if $clause (then $then) (else $else)) + /// (if ($clause)... (then $then) (else $else)) /// ``` /// - /// but it turns out we practically see a few things in the wild: - /// - /// * inside the `(if ...)` every sub-thing is surrounded by parens - /// * The `then` and `else` keywords are optional - /// * The `$then` and `$else` blocks don't need to be surrounded by parens - /// - /// That's all attempted to be handled here. The part about all sub-parts - /// being surrounded by `(` and `)` means that we hook into the `LParen` - /// parsing above to call this method there unconditionally. + /// THis method is called after a `(` is parsed within the `(if ...` block. + /// This determines what to do next. /// /// Returns `true` if the rest of the arm above should be skipped, or /// `false` if we should parse the next item as an instruction (because we @@ -309,53 +299,37 @@ impl<'a> ExpressionParser<'a> { _ => return Ok(false), }; - // The first thing parsed in an `if` statement is the clause. If the - // clause starts with `then`, however, then we know to skip the clause - // and fall through to below. - if let If::Clause(if_instr) = i { - let instr = mem::replace(if_instr, Instruction::End(None)); - *i = If::Then(instr); - if !parser.peek::()? { - return Ok(false); - } - } - - // All `if` statements are required to have a `then`. This is either the - // second s-expr (with or without a leading `then`) or the first s-expr - // with a leading `then`. The optionality of `then` isn't strictly what - // the text spec says but it matches wabt for now. - // - // Note that when we see the `then`, that's when we actually add the - // original `if` instruction to the stream. - if let If::Then(if_instr) = i { - let instr = mem::replace(if_instr, Instruction::End(None)); - self.instrs.push(instr); - *i = If::Else; - if parser.parse::>()?.is_some() { + match i { + // If the clause is still being parsed then interpret this `(` as a + // folded instruction unless it starts with `then`, in which case + // this transitions to the `Then` state and a new level has been + // reached. + If::Clause(if_instr) => { + if !parser.peek::()? { + return Ok(false); + } + parser.parse::()?; + let instr = mem::replace(if_instr, Instruction::End(None)); + self.instrs.push(instr); + *i = If::Then; self.stack.push(Level::IfArm); - return Ok(true); + Ok(true) } - return Ok(false); - } - // effectively the same as the `then` parsing above - if let If::Else = i { - self.instrs.push(Instruction::Else(None)); - if parser.parse::>()?.is_some() { - if parser.is_empty() { - self.instrs.pop(); - } + // Previously we were parsing the `(then ...)` clause so this next + // `(` must be followed by `else`. + If::Then => { + parser.parse::()?; + self.instrs.push(Instruction::Else(None)); + *i = If::Else; self.stack.push(Level::IfArm); - return Ok(true); + Ok(true) } - *i = If::End; - return Ok(false); - } - // If we made it this far then we're at `If::End` which means that there - // were too many s-expressions inside the `(if)` and we don't want to - // parse anything else. - Err(parser.error("unexpected token: too many payloads inside of `(if)`")) + // If after a `(else ...` clause is parsed there's another `(` then + // that's not syntactically allowed. + If::Else => Err(parser.error("unexpected token: too many payloads inside of `(if)`")), + } } /// Handles parsing of a `try` statement. A `try` statement is simpler diff --git a/crates/wit-component/tests/components/adapt-inject-stack-with-adapt-realloc/adapt-old.wat b/crates/wit-component/tests/components/adapt-inject-stack-with-adapt-realloc/adapt-old.wat index 5fc81bcbac..403204da2d 100644 --- a/crates/wit-component/tests/components/adapt-inject-stack-with-adapt-realloc/adapt-old.wat +++ b/crates/wit-component/tests/components/adapt-inject-stack-with-adapt-realloc/adapt-old.wat @@ -25,7 +25,7 @@ ;; `wit-component` should have injected a call to a function that allocates ;; the stack and sets $allocation_state to 2 - (if (i32.ne (global.get $allocation_state) (i32.const 2)) (unreachable)) + (if (i32.ne (global.get $allocation_state) (i32.const 2)) (then (unreachable))) ;; First, allocate a page using $cabi_realloc and write to it. This tests ;; that we can use the main module's allocator if present (or else a diff --git a/crates/wit-component/tests/components/adapt-inject-stack-with-realloc/adapt-old.wat b/crates/wit-component/tests/components/adapt-inject-stack-with-realloc/adapt-old.wat index 5fc81bcbac..403204da2d 100644 --- a/crates/wit-component/tests/components/adapt-inject-stack-with-realloc/adapt-old.wat +++ b/crates/wit-component/tests/components/adapt-inject-stack-with-realloc/adapt-old.wat @@ -25,7 +25,7 @@ ;; `wit-component` should have injected a call to a function that allocates ;; the stack and sets $allocation_state to 2 - (if (i32.ne (global.get $allocation_state) (i32.const 2)) (unreachable)) + (if (i32.ne (global.get $allocation_state) (i32.const 2)) (then (unreachable))) ;; First, allocate a page using $cabi_realloc and write to it. This tests ;; that we can use the main module's allocator if present (or else a diff --git a/crates/wit-component/tests/components/adapt-inject-stack-with-reallocing-adapter/adapt-old.wat b/crates/wit-component/tests/components/adapt-inject-stack-with-reallocing-adapter/adapt-old.wat index 4d5b998b5e..6626b29b7c 100644 --- a/crates/wit-component/tests/components/adapt-inject-stack-with-reallocing-adapter/adapt-old.wat +++ b/crates/wit-component/tests/components/adapt-inject-stack-with-reallocing-adapter/adapt-old.wat @@ -24,7 +24,7 @@ ;; `wit-component` should have injected a call to a function that allocates ;; the stack and sets $allocation_state to 2 - (if (i32.ne (global.get $allocation_state) (i32.const 2)) (unreachable)) + (if (i32.ne (global.get $allocation_state) (i32.const 2)) (then (unreachable))) ;; Allocate 8 bytes of stack space for the two u32 return values. The ;; original stack pointer is saved in local 0 and the stack frame for this diff --git a/tests/local/fuzz1.wat b/tests/local/fuzz1.wat index 576763f19b..a15ba7ec96 100644 --- a/tests/local/fuzz1.wat +++ b/tests/local/fuzz1.wat @@ -260,9 +260,9 @@ (i32.eqz (global.get $hangLimit) ) - (return + (then (return (i64.const 4293531749) - ) + )) ) (global.set $hangLimit (i32.sub @@ -279,8 +279,8 @@ (local.tee $0 (if (result i32) (i32.const -2147483647) - (local.get $0) - (block $label$4 (result i32) + (then (local.get $0)) + (else (block $label$4 (result i32) (br_if $label$4 (if (result i32) (block $label$5 (result i32) @@ -293,9 +293,9 @@ (i32.eqz (global.get $hangLimit) ) - (return + (then (return (i64.const -32766) - ) + )) ) (global.set $hangLimit (i32.sub @@ -315,22 +315,22 @@ ) ) ) - (block $label$7 (result f32) + (then (block $label$7 (result f32) (f32.const -nan:0x7fffda) - ) - (f32.const 4294967296) + )) + (else (f32.const 4294967296)) ) ) (i32.const -2147483648) ) - (i32.const -1073741824) - (local.get $0) + (then (i32.const -1073741824)) + (else (local.get $0)) ) (i32.eqz (global.get $global$0) ) ) - ) + )) ) ) ) diff --git a/tests/local/if-else-parsing.wast b/tests/local/if-else-parsing.wast new file mode 100644 index 0000000000..98a8bb60c6 --- /dev/null +++ b/tests/local/if-else-parsing.wast @@ -0,0 +1,42 @@ +(module + (func $a1 + i32.const 0 + (if (then) (else)) + ) + (func $a2 + (if (i32.const 1) (i32.eqz) (then) (else)) + ) + (func $a3 + (if (i32.const 1) (i32.eqz) (then)) + ) + (func $a4 + (if (i32.const 1) (i32.eqz) (then nop)) + ) +) +(assert_invalid + (module quote + "(func (if))" + ) + "no `then`") + +(assert_invalid + (module quote + "(func (if (else)))" + ) + "no `then`") + +(assert_invalid + (module quote + "(func (if nop (else)))" + ) + "expected `(`") +(assert_invalid + (module quote + "(func (if (nop) (else)))" + ) + "no `then`") +(assert_invalid + (module quote + "(func (if (nop) nop (then)))" + ) + "expected `(`") diff --git a/tests/snapshots/local/if-else-parsing.wast/0.print b/tests/snapshots/local/if-else-parsing.wast/0.print new file mode 100644 index 0000000000..56cbd49587 --- /dev/null +++ b/tests/snapshots/local/if-else-parsing.wast/0.print @@ -0,0 +1,29 @@ +(module + (type (;0;) (func)) + (func $a1 (;0;) (type 0) + i32.const 0 + if ;; label = @1 + else + end + ) + (func $a2 (;1;) (type 0) + i32.const 1 + i32.eqz + if ;; label = @1 + else + end + ) + (func $a3 (;2;) (type 0) + i32.const 1 + i32.eqz + if ;; label = @1 + end + ) + (func $a4 (;3;) (type 0) + i32.const 1 + i32.eqz + if ;; label = @1 + nop + end + ) +) \ No newline at end of file diff --git a/tests/snapshots/testsuite/if.wast/0.print b/tests/snapshots/testsuite/if.wast/0.print index 8e72869ed5..148a4b8282 100644 --- a/tests/snapshots/testsuite/if.wast/0.print +++ b/tests/snapshots/testsuite/if.wast/0.print @@ -22,12 +22,14 @@ end local.get 0 if ;; label = @1 + else end local.get 0 if $l ;; label = @1 end local.get 0 if $l ;; label = @1 + else end ) (func (;2;) (type 5) (param i32) (result i32) diff --git a/tests/snapshots/testsuite/proposals/function-references/if.wast/0.print b/tests/snapshots/testsuite/proposals/function-references/if.wast/0.print index 8e72869ed5..148a4b8282 100644 --- a/tests/snapshots/testsuite/proposals/function-references/if.wast/0.print +++ b/tests/snapshots/testsuite/proposals/function-references/if.wast/0.print @@ -22,12 +22,14 @@ end local.get 0 if ;; label = @1 + else end local.get 0 if $l ;; label = @1 end local.get 0 if $l ;; label = @1 + else end ) (func (;2;) (type 5) (param i32) (result i32) diff --git a/tests/snapshots/testsuite/proposals/gc/if.wast/0.print b/tests/snapshots/testsuite/proposals/gc/if.wast/0.print index 8e72869ed5..148a4b8282 100644 --- a/tests/snapshots/testsuite/proposals/gc/if.wast/0.print +++ b/tests/snapshots/testsuite/proposals/gc/if.wast/0.print @@ -22,12 +22,14 @@ end local.get 0 if ;; label = @1 + else end local.get 0 if $l ;; label = @1 end local.get 0 if $l ;; label = @1 + else end ) (func (;2;) (type 5) (param i32) (result i32)