Stream: git-wasmtime

Topic: wasmtime / Issue #1323 Cranelift: `cranelift_frontend::Sw...

Wasmtime GitHub notifications bot (Mar 14 2020 at 02:44):

thepowersgang opened Issue #1323:

Steps to reproduce:

• Structure code to seal blocks early (instead of sealing in bulk)
• Use cranelift_frontend::Switch to generate a sparse switch such that it generates chained comparisons instead of a jump table.
• Observe debug assertion regarding unsealed blocks.

Version:

• As of current HEAD (832666c45ead73fd833ea86da94cacb2f30a530c), there is no reference to seal_block in the switch module. Issue originally seen with v0.58 from crates.io

Wasmtime GitHub notifications bot (Mar 14 2020 at 02:44):

thepowersgang labeled Issue #1323:

Steps to reproduce:

• Structure code to seal blocks early (instead of sealing in bulk)
• Use cranelift_frontend::Switch to generate a sparse switch such that it generates chained comparisons instead of a jump table.
• Observe debug assertion regarding unsealed blocks.

Version:

• As of current HEAD (832666c45ead73fd833ea86da94cacb2f30a530c), there is no reference to seal_block in the switch module. Issue originally seen with v0.58 from crates.io

Wasmtime GitHub notifications bot (Mar 14 2020 at 02:44):

thepowersgang labeled Issue #1323:

Steps to reproduce:

• Structure code to seal blocks early (instead of sealing in bulk)
• Use cranelift_frontend::Switch to generate a sparse switch such that it generates chained comparisons instead of a jump table.
• Observe debug assertion regarding unsealed blocks.

Version:

• As of current HEAD (832666c45ead73fd833ea86da94cacb2f30a530c), there is no reference to seal_block in the switch module. Issue originally seen with v0.58 from crates.io

Wasmtime GitHub notifications bot (Mar 17 2020 at 04:29):

teapotd commented on Issue #1323:

Example:

let mut func = Function::new();
let mut builder_ctx = FunctionBuilderContext::new();
let mut builder = FunctionBuilder::new(&mut func, &mut builder_ctx);

let block0 = builder.create_block();
let block1 = builder.create_block();
let block2 = builder.create_block();
let block3 = builder.create_block();

builder.seal_block(block0);
builder.switch_to_block(block0);
let val = builder.ins().iconst(types::I32, 2);

let mut switch = Switch::new();
switch.set_entry(2, block1);
switch.set_entry(4, block2);
switch.emit(&mut builder, val, block3);

for &block in &[block1, block2, block3] {
    builder.seal_block(block);
    builder.switch_to_block(block);
    builder.ins().return_(&[]);
}

builder.finalize(); // panic: 'all blocks should be sealed before dropping a FunctionBuilder'
dbg!(func);

This panics in finalize, because Switch generates additional blocks and it doesn't seal them:

function u0:0() fast {
block0:
    v0 = iconst.i32 2
    v1 = icmp_imm eq v0, 4
    brnz v1, block2
    jump block4

block4: ; this block is not sealed
    v2 = icmp_imm.i32 eq v0, 2
    brnz v2, block1
    jump block3

block1:
    return

block2:
    return

block3:
    return
}

An obvious way to fix this issue would be to seal generated blocks when switch is built, but it would break code that seal sections in bulk using seal_all_blocks:

let mut func = Function::new();
let mut builder_ctx = FunctionBuilderContext::new();
let mut builder = FunctionBuilder::new(&mut func, &mut builder_ctx);

let block0 = builder.create_block();
builder.seal_block(block0);
builder.seal_all_blocks(); // panic: 'Attempting to seal block0 which is already sealed.'

We could change seal_all_blocks to ignore already sealed blocks instead of panicking.

Wasmtime GitHub notifications bot (Mar 19 2020 at 14:07):

thepowersgang commented on Issue #1323:

I feel that having Switch::emit (configurably) seal created blocks is the more correct option from a "user" point of view. Needing to call seal_all_blocks just because you used a Switch prevents you from catching cases where you forgot to seal one of your own blocks.

Wasmtime GitHub notifications bot (Apr 29 2020 at 18:42):

abrown closed Issue #1323:

Steps to reproduce:

• Structure code to seal blocks early (instead of sealing in bulk)
• Use cranelift_frontend::Switch to generate a sparse switch such that it generates chained comparisons instead of a jump table.
• Observe debug assertion regarding unsealed blocks.

Version:

• As of current HEAD (832666c45ead73fd833ea86da94cacb2f30a530c), there is no reference to seal_block in the switch module. Issue originally seen with v0.58 from crates.io

Last updated: Oct 23 2024 at 20:03 UTC