bytecodealliance / wasmtime

A fast and secure runtime for WebAssembly
https://wasmtime.dev/
Apache License 2.0
15.13k stars 1.27k forks source link

Cranelift: build_value_labels_ranges makes several incorrect assumptions #1362

Open bjorn3 opened 4 years ago

bjorn3 commented 4 years ago

All of those assumptions are broken by cg_clif, meaning that build_value_labels_ranges often misses values in the output.

github-actions[bot] commented 4 years ago

Subscribe to Label Action

This issue or pull request has been labeled: "cranelift"

Users Subscribed to "cranelift" * @bnjbvr

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

bjorn3 commented 4 years ago

I started working on this at https://github.com/bjorn3/wasmtime/tree/value_debuginfo_fixes, but I am stuck ATM. Associated cg_clif branch: https://github.com/bjorn3/rustc_codegen_cranelift/tree/wip_debuginfo_improvements (bjorn3/rustc_codegen_cranelift@12b6e83a489180ff52a9376c6674202c6cd1ecda). In the following case val0, val1, val2, val3, val4 and val5 should get one or more ValueLocRange's. However only val2 and val4 get one.

Debug println ``` { ComparableSourceLoc(SourceLoc(1)): [ (v3, val2), (v3, val5), ], ComparableSourceLoc(SourceLoc(2)): [ (v3, val5), (v6, val4), ], ComparableSourceLoc(SourceLoc(3)): [ (v3, val5), ], ComparableSourceLoc(SourceLoc(4)): [ (v6, val4), ], ComparableSourceLoc(SourceLoc(5)): [ (v7, val0), ], ComparableSourceLoc(SourceLoc(6)): [ (v7, val0), ], ComparableSourceLoc(SourceLoc(4294967294)): [ (v1, val2), (v2, val3), (v0, val1), ], } Instance { def: Item(DefId(0:34 ~ mini_core_hello_world[317d]::start[0])), substs: [()] } { val2: [ValueLocRange { loc: Reg(22), start: 5, end: 21 }], val4: [ValueLocRange { loc: Reg(16), start: 21, end: 27 }], } Some({ v0: Starts([ValueLabelStart { from: SourceLoc(4294967294), label: val1 }]), v1: Starts([ValueLabelStart { from: SourceLoc(4294967294), label: val2 }]), v2: Starts([ValueLabelStart { from: SourceLoc(4294967294), label: val3 }]), v3: Starts([ValueLabelStart { from: SourceLoc(1), label: val2 }, ValueLabelStart { from: SourceLoc(1), label: val5 }, ValueLabelStart { from: SourceLoc(2), label: val5 }, ValueLabelStart { from: SourceLoc(3), label: val5 }]), v6: Starts([ValueLabelStart { from: SourceLoc(2), label: val4 }, ValueLabelStart { from: SourceLoc(4), label: val4 }]), v7: Starts([ValueLabelStart { from: SourceLoc(5), label: val0 }, ValueLabelStart { from: SourceLoc(6), label: val0 }]) }) VarDebugInfo { name: "main", place: _1 } VarDebugInfo { name: "argc", place: _2 } VarDebugInfo { name: "argv", place: _3 } ```
Cranelift ir ``` test compile set is_pic target x86_64-unknown-linux-gnu function u0:0(i64 [%rdi], i64 [%rsi], i64 [%rdx], i64 fp [%rbp]) -> i64 [%rax], i64 fp [%rbp] system_v { ; symbol _ZN21mini_core_hello_world5start17hec55b7ca64fc434eE ; instance Instance { def: Item(DefId(0:34 ~ mini_core_hello_world[317d]::start[0])), substs: [()] } ; sig ([fn(), isize, *const *const u8]; c_variadic: false)->isize ; kind loc.idx param pass mode ty ; ssa _0 isize 8b 8, 8 ; ret _0 - ByVal(types::I64) isize ; arg _1 = v0 ByVal(types::I64) fn() ; arg _2 = v1 ByVal(types::I64) isize ; arg _3 = v2 ByVal(types::I64) *const *const u8 ; kind local ty size align (abi,pref) ; ssa _1 fn() 8b 8, 8 ; ssa _2 isize 8b 8, 8 ; ssa _3 *const *const u8 8b 8, 8 ; ssa _4 bool 1b 1, 1 ; ssa _5 isize 8b 8, 8 ss0 = incoming_arg 16, offset -16 block0(v0: i64 [%rdi], v1: i64 [%rsi], v2: i64 [%rdx], v8: i64 [%rbp]): v3 -> v1 [RexOp1pushq#50] x86_push v8 [RexOp1copysp#8089] copy_special %rsp -> %rbp ; ; val2@%rsi @fffffffe [-] nop ; write_cvalue: Var(_1) <- ByVal(v0) ; write_cvalue: Var(_2) <- ByVal(v1) ; write_cvalue: Var(_3) <- ByVal(v2) @fffffffe [-] fallthrough block1 block1: @fffffffe [-] nop ; StorageLive(_4) ; StorageLive(_5) ; _5 = _2 ; write_cvalue: Var(_5) <- ByVal(v3) ; _4 = Eq(move _5, const 3isize) @0002 [RexOp1pu_id#b8,%rax] v4 = iconst.i64 3 @0002 [DynRexOp1icscc#8039,%rax] v5 = icmp.i64 eq v1, v4 @0002 [RexOp2urm_noflags#4b6,%rax] v6 = bint.i8 v5 ; write_cvalue: Var(_4) <- ByVal(v6) ; StorageDead(_5) ; ; val4@%rax val2☠ @0003 [ghost_use_gpr#00] ghost_use.i64 v1 ; StorageDead(_4) @0004 [ghost_use_gpr#00] ghost_use v6 ; _0 = const 0isize @0005 [RexOp1pu_id#b8,%rax] v7 = iconst.i64 0 ; write_cvalue: Var(_0) <- ByVal(v7) ; ; return ; ; val4☠ [RexOp1popq#58,%rbp] v9 = x86_pop.i64 @0006 [Op1ret#c3] return v7, v9 } ```
bjorn3 commented 4 years ago
[
    LiveRange { value: v0, affinity: Reg(RegClassIndex(0)), def_begin: ProgramPoint(block0), def_end: ProgramPoint(inst1), liveind: [Interval { begin: block1, end: inst6 }] },
    LiveRange { value: v2, affinity: Reg(RegClassIndex(0)), def_begin: ProgramPoint(inst3), def_end: ProgramPoint(inst4), liveind: [] },
    LiveRange { value: v3, affinity: Reg(RegClassIndex(4)), def_begin: ProgramPoint(inst4), def_end: ProgramPoint(inst5), liveind: [] },
    LiveRange { value: v4, affinity: Reg(RegClassIndex(0)), def_begin: ProgramPoint(inst5), def_end: ProgramPoint(inst7), liveind: [] },
]

It seems that v1 doesn't have liveness information despite the ghost_use.i64 v1.

yurydelendik commented 4 years ago

It assumes SourceLoc is strictly ascending. (Checking for last_srcloc.unwrap() > srcloc instead of last_srcloc.unwrap() != srcloc)

This property might be essential track ranges transformation (e.g. during DWARF translation). Though it is possible to expose that indirectly via some context.