WebAssembly / binaryen

Optimizer and compiler/toolchain library for WebAssembly
Apache License 2.0
7.31k stars 717 forks source link

wasm-split confused about overlapping segments #6693

Open kripken opened 1 month ago

kripken commented 1 month ago

(At least that is my guess as to what is going wrong here, I'm not sure.)

Testcase:

(module
 (type $0 (func))
 (type $1 (func (result i32)))
 (type $2 (func (param f64 i64 f32) (result f64)))                                                                                                                                            
 (type $3 (func (param i64 f32 i32) (result i32)))
 (type $4 (func (result i64)))
 (type $5 (func (result f32)))                                                                 
 (type $6 (func (param i64 i64) (result f64)))
 (type $7 (func (param i32 f64 i32 f32 f64) (result f64)))
 (type $8 (func (param i32 i64) (result f64)))                                                 
 (type $9 (func (param f64 f32 i64 f32 i32)))
 (type $10 (func (param f64 i32 f32 i32 f64) (result i64)))                                                                                                                                   
 (global $global$0 (mut i32) (i32.const 0))
 (global $global$1 (mut i64) (i64.const 0))
 (global $global$2 (mut f64) (f64.const 0))
 (global $global$3 (mut f32) (f32.const 0))
 (global $global$4 (mut f32) (f32.const 0))
 (global $global$5 (mut f64) (f64.const 0))                                                    
 (global $global$6 (mut i32) (i32.const 0))                                                    
 (memory $0 16 17)                                                                             
 (table $0 22 funcref)                                                                         
 (elem $0 (i32.const 0))                                                                       
 (elem $1 (i32.const 0))                                                                       
 (elem $2 (i32.const 0) $0 $0 $0 $0 $0 $0 $0 $11 $5 $4 $7)
 (elem $3 (i32.const 1) $0 $0 $0 $0 $0 $0 $4 $2 $4 $8)
 (elem $4 (i32.const 0))            
 (export "func_141" (func $7))
 (func $0      
 )
 (func $1 (result i32)
  (unreachable)
 )
 (func $2 (param $0 f64) (param $1 i64) (param $2 f32) (result f64)
  (unreachable)
 )
 (func $3 (param $0 i64) (param $1 f32) (param $2 i32) (result i32)                            
  (unreachable)
 )                                                                                                                                                                                            
 (func $4 (result i64)
  (unreachable)
 )                                                                                             
 (func $5 (result f32)
  (unreachable)
 )
 (func $6 (param $0 i64) (param $1 i64) (result f64)                                           
  (unreachable)                                                                                                                                                                               
 )     
 (func $7        
 )                            
 (func $8 (param $0 i32) (param $1 f64) (param $2 i32) (param $3 f32) (param $4 f64) (result f64)                                                                                             
  (unreachable)                                                                                
 )                            
 (func $9 (param $0 i32) (param $1 i64) (result f64)                                           
  (unreachable)                               
 )                                                                                             
 (func $10 (param $0 f64) (param $1 f32) (param $2 i64) (param $3 f32) (param $4 i32)          
 )                                           
 (func $11 (param $0 f64) (param $1 i32) (param $2 f32) (param $3 i32) (param $4 f64) (result i64)                                                                                            
  (unreachable)                            
 )                                         
)                                          

The testcase doesn't do much: one export, which is an empty function with no params or results. After splitting it crashes, though.

Split command:

bin/wasm-split a.wat --split --split-funcs 4,5,6,7,8,9 --primary-output b.wasm --secondary-output c.wasm

Primary module:

(module
 (type $0 (func))
 (type $1 (func (result i64)))
 (type $2 (func (result f32)))
 (type $3 (func (param i32 f64 i32 f32 f64) (result f64)))
 (type $4 (func (result i32)))
 (type $5 (func (param f64 i64 f32) (result f64)))
 (type $6 (func (param i64 f32 i32) (result i32)))
 (type $7 (func (param f64 f32 i64 f32 i32)))
 (type $8 (func (param f64 i32 f32 i32 f64) (result i64)))
 (import "placeholder" "8" (func $fimport$0 (result f32)))
 (import "placeholder" "9" (func $fimport$1 (result i64)))
 (import "placeholder" "10" (func $fimport$2))
 (import "placeholder" "7" (func $fimport$3 (result i64)))
 (import "placeholder" "9" (func $fimport$4 (result i64)))
 (import "placeholder" "10" (func $fimport$5 (param i32 f64 i32 f32 f64) (result f64)))
 (global $global$0 (mut i32) (i32.const 0))
 (global $global$1 (mut i64) (i64.const 0))
 (global $global$2 (mut f64) (f64.const 0))
 (global $global$3 (mut f32) (f32.const 0))
 (global $global$4 (mut f32) (f32.const 0))
 (global $global$5 (mut f64) (f64.const 0))
 (global $global$6 (mut i32) (i32.const 0))
 (memory $0 16 17)
 (table $0 22 funcref)
 (elem $0 (i32.const 0))
 (elem $1 (i32.const 0))
 (elem $2 (i32.const 0) $0 $0 $0 $0 $0 $0 $0 $5 $fimport$0 $fimport$1 $fimport$2)
 (elem $3 (i32.const 1) $0 $0 $0 $0 $0 $0 $fimport$3 $2 $fimport$4 $fimport$5)
 (elem $4 (i32.const 0))
 (export "func_141" (func $6))
 (export "memory" (memory $0))
 (export "table" (table $0))
 (export "global" (global $global$0))
 (export "global_4" (global $global$1))
 (export "global_5" (global $global$2))
 (export "global_6" (global $global$3))
 (export "global_7" (global $global$4))
 (export "global_8" (global $global$5))
 (export "global_9" (global $global$6))
 (func $0
  (nop)
 )
 (func $1 (result i32)
  (unreachable)
 )
 (func $2 (param $0 f64) (param $1 i64) (param $2 f32) (result f64)
  (unreachable)
 )
 (func $3 (param $0 i64) (param $1 f32) (param $2 i32) (result i32)
  (unreachable)
 )
 (func $4 (param $0 f64) (param $1 f32) (param $2 i64) (param $3 f32) (param $4 i32)
  (nop)
 )
 (func $5 (param $0 f64) (param $1 i32) (param $2 f32) (param $3 i32) (param $4 f64) (result i64)
  (unreachable)
 )
 (func $6
  (call_indirect (type $0)
   (i32.const 10)
  )
 )
)

This already looks odd. The exported function (now $6) does a call_indirect at index 10. There are two elems that write that index, that overlap. One writes $fimport$2 and the other $fimport$5. The last one wins. But $fimport$5 has a very different type than $0 so the VM errors.

But there might be more going wrong here than overlapping segments. This is the secondary module:

(module
 (type $0 (func (result i64)))
 (type $1 (func (result f32)))
 (type $2 (func (param i64 i64) (result f64)))
 (type $3 (func))
 (type $4 (func (param i32 f64 i32 f32 f64) (result f64)))
 (type $5 (func (param i32 i64) (result f64)))
 (import "primary" "memory" (memory $mimport$0 16 17))
 (import "primary" "table" (table $timport$0 22 funcref))
 (import "primary" "global" (global $gimport$0 (mut i32)))
 (import "primary" "global_4" (global $gimport$1 (mut i64)))
 (import "primary" "global_5" (global $gimport$2 (mut f64)))
 (import "primary" "global_6" (global $gimport$3 (mut f32)))
 (import "primary" "global_7" (global $gimport$4 (mut f32)))
 (import "primary" "global_8" (global $gimport$5 (mut f64)))
 (import "primary" "global_9" (global $gimport$6 (mut i32)))
 (elem $0 (i32.const 7) $0 $1 $0 $4)
 (func $0 (result i64)
  (unreachable)
 )
 (func $1 (result f32)
  (unreachable)
 )
 (func $2 (param $0 i64) (param $1 i64) (result f64)
  (unreachable)
 )
 (func $3
  (nop)
 )
 (func $4 (param $0 i32) (param $1 f64) (param $2 i32) (param $3 f32) (param $4 f64) (result f64)
  (unreachable)
 )
 (func $5 (param $0 i32) (param $1 i64) (result f64)
  (unreachable)
 )
)

It writes function $4 to index 10, but it has the weird signature again, so that is also wrong.

@tlively is this a known issue?

tlively commented 1 month ago

wasm-split assumes that the module (and in particular the table and element segments) don't look too different from a normal statically linked or shared library emitted by LLVM and wasm-ld. I'm not surprised that things go wrong when there are overlapping active segments. The best fix is probably to implement this TODO to use a fresh table that cannot possibly interfere with the original program when reference-types are enabled, then fuzz with reference-types unconditionally enabled.

kripken commented 1 month ago

wasm-split assumes that the module (and in particular the table and element segments) don't look too different from a normal statically linked or shared library emitted by LLVM and wasm-ld.

Do you think there might be many more such assumptions? This one sounds easy enough to handle, but if there is a lot then maybe this is not worth doing. The context for me here is to use wasm-split + wasm2js to fuzz the wasm/JS boundary, but that only makes sense if there isn't too much work to get both of those things fuzzable.

tlively commented 1 month ago

No, table management should be the only problem area since we previously supported only one table and wasm-split needed to modify it. Memories are similarly a problem area without multi-memory, but only for multithreaded profiling, so I don't think that will be a problem for you.

kripken commented 1 month ago

Ok, I tried to implement that TODO here:

https://github.com/WebAssembly/binaryen/compare/main...kripken:binaryen:split.new.table?expand=1

But the results aren't right, and I think there is some underlying assumption or bug in code I am not familiar with. The new table that is created is of size 0, and if there is nothing to put there in the primary module then it stays at size 0, but if the secondary module needs to put something there then it tries to add to a size-0 table, which errors. That is, when the secondary module needs more space, the table initial size (declared in both modules) must be larger, but that isn't happening.

I tried to look into this but I saw that TableSlotManager::getSlot is not called for secondary module operations, so I'm not sure where in the code to look for growing the table in response to secondary module needs?

tlively commented 1 month ago

I can take a deeper look at this.

tlively commented 2 weeks ago

6726 should fix this problem when reference-types are enabled. Is that enough to close this issue?