From 2674545bf90c602a278ef133098fa05b7613227e Mon Sep 17 00:00:00 2001 From: Stephan Renatus Date: Tue, 1 Feb 2022 17:48:03 +0100 Subject: [PATCH] planner: if dynamic call fails, or deref of data fails, break to undefined (#4275) Due to excessive nesting of blocks, the break statement wasn't breaking out of enough of them: this would this add an element to the result set when it should have turned into undefined. This was the case in two conditions: a. the dynamic lookup not finding a data function to call, and the resulting static data lookup failing to resolve the ref; or b. the dynamically called function returns undefined Signed-off-by: Stephan Renatus --- internal/compiler/wasm/wasm.go | 6 +- internal/planner/planner.go | 37 +++++----- internal/wasm/sdk/test/e2e/external_test.go | 1 + .../019_call_indirect_optimization.yaml | 70 ++++++++++++++++++- 4 files changed, 93 insertions(+), 21 deletions(-) diff --git a/internal/compiler/wasm/wasm.go b/internal/compiler/wasm/wasm.go index 7e8048bfd0..c5198f2f76 100644 --- a/internal/compiler/wasm/wasm.go +++ b/internal/compiler/wasm/wasm.go @@ -1074,8 +1074,8 @@ func (c *Compiler) compileBlock(block *ir.Block) ([]instruction.Instruction, err instrs = append(instrs, instruction.I32Eqz{}) instrs = append(instrs, instruction.BrIf{Index: 0}) } else { - // Booleans and strings would lead to the BrIf (since opa_value_get - // on them returns 0), so let's skip that. + // Booleans and string sources would lead to the BrIf (since opa_value_get + // on them returns 0), so let's skip trying that. instrs = append(instrs, instruction.Br{Index: 0}) break } @@ -1484,7 +1484,7 @@ func (c *Compiler) compileCallDynamicStmt(stmt *ir.CallDynamicStmt, result *[]in instruction.CallIndirect{Index: typeIndex}, // [arg0 arg1 tbl_idx] -> [res] instruction.TeeLocal{Index: c.local(stmt.Result)}, instruction.I32Eqz{}, - instruction.BrIf{Index: 2}, // mapping found, "undefined" result counts + instruction.BrIf{Index: 3}, // mapping found, "undefined" result counts ) *result = append(*result, instrs...) diff --git a/internal/planner/planner.go b/internal/planner/planner.go index b982999ac7..b9c5b4b2d9 100644 --- a/internal/planner/planner.go +++ b/internal/planner/planner.go @@ -1473,23 +1473,26 @@ func (p *Planner) planRefData(virtual *ruletrie, base *baseptr, ref ast.Ref, ind // We're planning a structure like this: // - // block a - // block b - // block c1 - // opa_mapping_lookup || br c1 - // call_indirect || br a - // br b + // block res + // block a + // block b + // block c1 + // opa_mapping_lookup || br c1 + // call_indirect || br res + // br b + // end + // block c2 + // dot i || br c2 + // dot i+1 || br c2 + // br b + // end + // br a // end - // block c2 - // dot i || br c2 - // dot i+1 || br c2 - // br b - // end - // br a - // end - // dot i+2 || br a - // dot i+3 || br a - // end + // dot i+2 || br res + // dot i+3 || br res + // end; a + // [add_to_result_set] + // end; res // // We have to do it like this because the dot IR stmts // are compiled to `br 0`, the innermost block, if they @@ -1531,7 +1534,7 @@ func (p *Planner) planRefData(virtual *ruletrie, base *baseptr, ref ast.Ref, ind { // block "b" in the sketch above Stmts: []ir.Stmt{ &ir.BlockStmt{Blocks: []*ir.Block{callDynBlock, dotBlock}}, - &ir.BreakStmt{Index: 1}}, + &ir.BreakStmt{Index: 2}}, }, }}, }} diff --git a/internal/wasm/sdk/test/e2e/external_test.go b/internal/wasm/sdk/test/e2e/external_test.go index 15b3d7ac41..4dee70bf82 100644 --- a/internal/wasm/sdk/test/e2e/external_test.go +++ b/internal/wasm/sdk/test/e2e/external_test.go @@ -2,6 +2,7 @@ // Use of this source code is governed by an Apache2 // license that can be found in the LICENSE file. +//go:build wasm_sdk_e2e // +build wasm_sdk_e2e package e2e diff --git a/test/wasm/assets/019_call_indirect_optimization.yaml b/test/wasm/assets/019_call_indirect_optimization.yaml index 596e3f4a7b..afe3fb906b 100644 --- a/test/wasm/assets/019_call_indirect_optimization.yaml +++ b/test/wasm/assets/019_call_indirect_optimization.yaml @@ -120,4 +120,72 @@ cases: q: baz: 8 want_result: - - x: 100 \ No newline at end of file + - x: 100 + - note: func lookup unsuccessful, data deref unsuccessful + query: data.test.p = x + modules: + - | + package test + + p { + x := "foo" + data.other[x].p + } + - | + package other.bar # not "foo", but existence triggers dynamic lookup + + p = true + data: {} + want_defined: false + - note: func lookup successful, but call yields undefined + query: data.test.p = x + modules: + - | + package test + + p { + x := "foo" + data.other[x].p + } + - package other.foo # empty + - | + package other.bar # not foo + + p = true + data: {} + want_defined: false + - note: func lookup successful, but call yields undefined (in a loop) + query: data.test.p = x + modules: + - | + package test + xs := {"foo", "baz", "123"} + p { + xs[x] + data.other[x].p + } + - package other.foo # empty + - | + package other.bar # not foo + + p = true + data: {} + want_defined: false + - note: func lookup in a loop, one call yields a result + query: data.test.p = x + modules: + - | + package test + xs := {"fox", "baz", "123", "xyz"} + p = y { + xs[x] + y := data.other[x].p + } + - package other.foo # empty + - | + package other.xyz + + p = "yay" + data: {} + want_result: + - x: yay