Stream: git-wasmtime

Topic: wasmtime / PR #2357 cranelift: refactor unwind logic to a...

Wasmtime GitHub notifications bot (Nov 03 2020 at 23:12):

yurydelendik requested peterhuene for a review on PR #2357.

Wasmtime GitHub notifications bot (Nov 03 2020 at 23:12):

yurydelendik opened PR #2357 from pub-unwindcode to main:

Prepares code for #2313

• Make cranelift_codegen::isa::unwind::input public
• Move UnwindCode's common offset field out of the structure
• Make MachCompileResult::unwind_info more generic

Wasmtime GitHub notifications bot (Nov 04 2020 at 14:01):

yurydelendik updated PR #2357 from pub-unwindcode to main:

Prepares code for #2313

• Make cranelift_codegen::isa::unwind::input public
• Move UnwindCode's common offset field out of the structure
• Make MachCompileResult::unwind_info more generic

Wasmtime GitHub notifications bot (Nov 04 2020 at 14:51):

yurydelendik updated PR #2357 from pub-unwindcode to main:

Prepares code for #2313

• Make cranelift_codegen::isa::unwind::input public
• Move UnwindCode's common offset field out of the structure
• Make MachCompileResult::unwind_info more generic

Wasmtime GitHub notifications bot (Nov 04 2020 at 17:40):

yurydelendik updated PR #2357 from pub-unwindcode to main:

Prepares code for #2313

• Make cranelift_codegen::isa::unwind::input public
• Move UnwindCode's common offset field out of the structure
• Make MachCompileResult::unwind_info more generic

Wasmtime GitHub notifications bot (Nov 04 2020 at 20:46):

yurydelendik updated PR #2357 from pub-unwindcode to main:

Prepares code for #2313

• Make cranelift_codegen::isa::unwind::input public
• Move UnwindCode's common offset field out of the structure
• Make MachCompileResult::unwind_info more generic

Wasmtime GitHub notifications bot (Nov 05 2020 at 13:33):

yurydelendik edited PR #2357 from pub-unwindcode to main:

Prepares code for #2313

• Make cranelift_codegen::isa::unwind::input public
• Move UnwindCode's common offset field out of the structure
• Make MachCompileResult::unwind_info more generic
• Record initial stack pointer offset (as in CIE)

The changes allows furthe introduction of fastcall logic and support of ARM-like architectures.

Wasmtime GitHub notifications bot (Nov 05 2020 at 14:23):

yurydelendik updated PR #2357 from pub-unwindcode to main:

Prepares code for #2313

• Make cranelift_codegen::isa::unwind::input public
• Move UnwindCode's common offset field out of the structure
• Make MachCompileResult::unwind_info more generic
• Record initial stack pointer offset (as in CIE)

The changes allows furthe introduction of fastcall logic and support of ARM-like architectures.

Wasmtime GitHub notifications bot (Nov 05 2020 at 19:46):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 19:46):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 19:46):

cfallin created PR Review Comment:

s/intermidate/intermediate/

Wasmtime GitHub notifications bot (Nov 05 2020 at 19:46):

cfallin created PR Review Comment:

s/speficied/specified/ (and below)

Wasmtime GitHub notifications bot (Nov 05 2020 at 19:46):

cfallin created PR Review Comment:

Outdated comment (not just one word-size but configurable)?

Wasmtime GitHub notifications bot (Nov 05 2020 at 19:46):

cfallin created PR Review Comment:

This is pretty counter-intuitive to me without more context: maybe a comment describing the scheme would help. I think what is going on is: if the call-frame address is already defined in terms of a frame register, then this call overrides that definition (Cfa sets both reg and offset); but if no frame register is defined, then this call defines the given register to be the frame register. Is that right?

I'm not sure I understand the semantics fully either. "Frame register" implies to me a register that does not move, and elsewhere in this impl, we conditionally adjust offsets based on whether or not we're using a frame register. It seems that whether or not we're using a frame register is determined by how many times we call set_cfa_reg (whether odd or even count, I think?), which is confusing to me.

Wasmtime GitHub notifications bot (Nov 05 2020 at 19:46):

cfallin created PR Review Comment:

Hmm -- long-term, I think it might make more sense to generate the necessary UnwindCode sequence directly in prologue/epilogue generation, rather than reverse-engineering the VCode sequence; this feels somewhat fragile (we have to keep in mind this translation if we change the ABI code). Out of scope for this PR, but I wonder if you have any thoughts on that...

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:25):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:25):

bjorn3 created PR Review Comment:

"Reliable and Fast DWARF-Based Stack Unwinding" (https://hal.inria.fr/hal-02297690/document), actually states the how hard it is to keep the unwind info in sync while performing optimizations as one of it's reasons to generate unwind info from the actual assembly.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:29):

peterhuene created PR Review Comment:

This is really strange to me as this rsp isn't really a frame pointer and if this is only necessary to incur a call to set_cfa_reg for popping rbp in SystemV unwind information generation then I'd prefer it done a different way.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:29):

peterhuene created PR Review Comment:

nit: should we keep this a constant?

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:29):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:29):

peterhuene created PR Review Comment:

Perhaps a filter_map here instead of returning an empty Vec for the last epilogue?

Also, if there's always two elements in the Vec, I wonder if UnwindInfo<Reg>::epilogues_unwind_codes should be Vec<[(CodeOffset, UnwindCode<Reg>); 2]> to reduce these small allocations?

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:29):

peterhuene created PR Review Comment:

I'm also having a hard time understanding why this is necessary from what it was previously implemented, namely recording a CFA change for the restore of rbp (i.e. in restore_reg) rather than having a strange, to me at least, "set frame pointer to rsp" unwind operation with these changes that causes the call to set_cfa_reg.

Yury, is there a semantic difference to the resulting baseline changes?

specifically:

DW_CFA_def_cfa(rsp, 8)
DW_CFA_same_value(rbp)

and

DW_CFA_same_value (rbp)
DW_CFA_def_cfa(rsp, 8)

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:29):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:30):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:35):

peterhuene created PR Review Comment:

The more I think about this, the more I wonder if UnwindInfo<Reg>::epilogues_unwind_codes is even necessary rather than letting the unwind format consume the epilogue ranges from UnwindInfoContext and handle the remember state / restore state logic (which is DWARF-specific anyway).

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:35):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:38):

peterhuene created PR Review Comment:

On an aside: do we have any test cases for DW_CFA_remember_state/DW_CFA_restore_state? A quick grep doesn't show any results.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:38):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:42):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:42):

yurydelendik created PR Review Comment:

DW_CFA_def_cfa(rsp, 8) and DW_CFA_same_value(rbp) affect different states, so in this case order is not relevant.

I'll add ResetFramePointer to restore CFA to its default value, as specified as the CIE.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:44):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:44):

peterhuene created PR Review Comment:

A different unwind operation (e.g. RestoreFramePointer) I think makes sense to me :+1:

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:52):

yurydelendik updated PR #2357 from pub-unwindcode to main:

Prepares code for #2313

• Make cranelift_codegen::isa::unwind::input public
• Move UnwindCode's common offset field out of the structure
• Make MachCompileResult::unwind_info more generic
• Record initial stack pointer offset (as in CIE)

The changes allows furthe introduction of fastcall logic and support of ARM-like architectures.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:57):

peterhuene created PR Review Comment:

Per the discussion in the other comment thread, I think this will be resolved with a new RestoreFramePointer unwind code.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:57):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:58):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:58):

yurydelendik created PR Review Comment:

It is a TODO -- added a comment about that. The test is test_multi_return_func at unwind/system.rs

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:58):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:59):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 20:59):

peterhuene created PR Review Comment:

Ah, missed that as I was looking for a file test.

Wasmtime GitHub notifications bot (Nov 05 2020 at 22:22):

yurydelendik updated PR #2357 from pub-unwindcode to main:

Prepares code for #2313

• Make cranelift_codegen::isa::unwind::input public
• Move UnwindCode's common offset field out of the structure
• Make MachCompileResult::unwind_info more generic
• Record initial stack pointer offset (as in CIE)

The changes allows furthe introduction of fastcall logic and support of ARM-like architectures.

Wasmtime GitHub notifications bot (Nov 05 2020 at 22:31):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 22:31):

cfallin created PR Review Comment:

Interesting; one thing to note is that the prologue generation in our case happens right before code emission, after any optimizations, but point taken that there is a potential for mismatch. I suppose as long as we are careful with any changes to the prologue code that the translation still works (though the tests look quite good here so that gives me confidence).

Wasmtime GitHub notifications bot (Nov 05 2020 at 22:32):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 22:57):

yurydelendik merged PR #2357.

Last updated: Dec 23 2024 at 12:05 UTC