Track frame layout changes. #679

yurydelendik · 2019-02-19T22:12:59Z

Adds basic frame layout (or unwinding) capabilities to the x86 abi.

bjorn3 · 2019-02-21T13:55:32Z

Using .debug_frame will hopefully fix gdb backtraces in debug mode. (gdb gets confused by rex push rbp https://github.com/bjorn3/rustc_codegen_cranelift/issues/146#issuecomment-449474527)

lachlansneff · 2019-03-11T22:21:59Z

It'd be useful to generate the .eh_frame and equivalent sections so that we can use the system unwinder.

cranelift-codegen/src/ir/function.rs

cranelift-codegen/src/isa/x86/abi.rs

philipc · 2019-04-20T05:20:38Z

It'd be useful to generate the .eh_frame

I've implemented write support for .eh_frame in gimli (gimli-rs/gimli#419). I expect that it should require no changes to this PR, and only a minor change to the wasmtime PR, but I haven't tested.

bnjbvr

A few high-level questions below, to clarify the purpose of this PR. Thanks!

bnjbvr · 2019-07-08T16:34:00Z

cranelift-codegen/src/isa/x86/abi.rs

+    let push_fp_inst = pos.ins().x86_push(fp);
+    let word_size = word_size as isize;
+    cfa_state.position -= word_size;
+    cfa_state.offset += word_size;


Are we sure no compiler pass won't add more instructions in between? (in particular thinking about register allocation which could insert copies/fills, or constant pools being generated, whenever we have them?)

the prologue_epilogue runs after regalloc but before shrink_instructions, relax_branches -- I hope the latter does not insert anything to change stack layout.

Right. There should be @julian-seward1 's late stage optimizations happening after that (and they can remove instructions), so we should be careful to put this somewhere the data flow graph can't change (i.e. at the very end).

cranelift-codegen/src/isa/x86/abi.rs

cranelift-codegen/src/ir/framelayout.rs

bnjbvr · 2019-07-08T16:56:46Z

cranelift-codegen/src/isa/x86/abi.rs

+        pos.func.frame_layout.instructions.insert(
+            reg_push_inst,
+            vec![FrameLayoutChange::RegAt {
+                reg: reg,


Are we interested in the position of every register? (I guess so, if I understand correctly this is for tracking where local variables are.) If so, should every instruction in the function's body also have its own frame layout changes (in case regalloc moves a variable from one register to another)?

With this PR (and .debug_frame) we are interested in basics: proper frame unwinding (i.e. BP, SP, IP) and maybe registers used for args. (The tracking of original variables values is happening somewhere else).

cranelift-codegen/src/ir/function.rs

cranelift-codegen/src/ir/framelayout.rs

bnjbvr · 2019-07-10T15:31:22Z

Deferring to @sunfishcode, since my understanding is that this is mostly for wasmtime.

bjorn3 · 2019-07-10T15:46:20Z

This is useful for cg_clif for debuginfo and unwinding too.

cranelift-codegen/src/isa/x86/abi.rs

sunfishcode · 2019-09-07T21:55:26Z

I think this patch looks good overall, though @iximeow's comment looks worth addressing.

iximeow · 2019-09-24T16:49:40Z

Just a note: the above merged PR adds frame changes to track the last few instructions as we exit functions on x86. I'd forgotten when commenting but we can have multiple returns, so it's a little more complex than I'd initially thought. I'm not sure how to get multi-return sys-v functions out of Cranelift to test, but to my understanding of the spec (and a comparison against gcc) it looks correct.

So much as my word matters here: these changes look ✔️

cranelift-codegen/src/ir/framelayout.rs

cranelift-codegen/src/isa/x86/abi.rs

cranelift-codegen/src/ir/framelayout.rs

iximeow · 2019-10-14T18:35:42Z

I just realized that I tried to re-request a review on this a few weeks ago, but it didn't go through. Maybe I can't re-request reviews for other folks' PRs? Anyway, if you have a moment @bjorn3 I'd like to get this one sorted out and resume getting #902 ship-shape soon.

bjorn3 · 2019-10-14T19:00:08Z

cranelift-codegen/src/isa/x86/abi.rs

+            .and_modify(|insts| {
+                *insts = insts
+                    .into_iter()
+                    .map(|x| *x)


bjorn3 · 2019-10-14T19:01:13Z

@iximeow I am not a member of @CraneStation.

iximeow · 2019-10-14T19:03:53Z

oh whoops, I'd thought you were. Sorry about that!

bnjbvr · 2019-10-21T09:30:46Z

Hi! Just as a preamble to the review, can you make sure this commit doesn't introduce merge commits please? (Rebasing should work just fine.)

bnjbvr · 2019-10-22T15:57:44Z

@yurydelendik Hey, can you help with this rebase, please? (Once that's done, I'm happy to review this.)

yurydelendik · 2019-10-22T18:24:16Z

can you help with this rebase

rebased all commits

bnjbvr

A few suggestions below, and a few questions about logic too. It should be pretty much ready to land after this. (I think we'll eventually squash commits on merge in this PR, since each commit tends to modify the behavior of its predecessors.)

bnjbvr · 2019-10-24T15:46:38Z

cranelift-codegen/src/ir/framelayout.rs

+#[cfg(not(feature = "std"))]
+use crate::HashMap;
+#[cfg(feature = "std")]
+use std::collections::HashMap;


nit: we don't need to guard against the std feature here and we can just use crate::HashMap;, right?

bnjbvr · 2019-10-24T15:46:58Z

cranelift-codegen/src/ir/framelayout.rs

+/// Change in the frame layout information.
+#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)]
+pub enum FrameLayoutChange {
+    /// Base CFA pointer moved to different register/offset.


Even if it's obvious from context here, would be nice to explicit CFA in this file at least once.

bnjbvr · 2019-10-24T15:47:26Z

cranelift-codegen/src/ir/framelayout.rs

+    /// The entire frame layout must be preserved somewhere to be restored at a corresponding
+    /// `Restore` change.
+    ///
+    /// This likely maps to the DWARF call frame instruction `.cfa_remember_state`


nit: can you end this sentence with a dot please? (ditto below for restore)

bnjbvr · 2019-10-24T15:50:06Z

cranelift-codegen/src/isa/x86/abi.rs

@@ -371,6 +372,29 @@ fn baldrdash_prologue_epilogue(func: &mut ir::Function, isa: &dyn TargetIsa) ->
    Ok(())
 }

+/// CFAState is `cranelift`'s model of the call frame layout at any particular point in a function.


It'd be nice to explicit the CFA acronym once in this file too.

bnjbvr · 2019-10-24T15:51:23Z

cranelift-codegen/src/isa/x86/abi.rs

+    cf_ptr_offset: isize,
+    /// The offset between the start of the call frame and the current stack pointer. This is
+    /// primarily useful to point to where on the stack preserved registers are, but is maintained
+    /// through the whole function for consistency.


This comment is slightly lying :) It is only preserved in the prologue and epilogue, as far as I can tell, not in the function's body itself. It would be nice to reword it a bit.

We dont have alloca support, so both esp and ebp arent modified between prologue and epilogue, right?

I was thinking about regalloc's spills, but maybe regalloc just mandates a specific amount of spill slots, and something else allocates them in advance once and for all?

Spill slots are a kind of stack slots. I believe they get allocated together with the rest of the stack slots in prologue.

And i imagine the emergency spill slot belongs to that group? In this case, current comment is fine.

Yes, it is a kind of stack slot: https://docs.rs/cranelift-codegen/0.46.1/cranelift_codegen/ir/stackslot/enum.StackSlotKind.html#variant.EmergencySlot

bnjbvr · 2019-10-24T15:57:41Z

cranelift-codegen/src/isa/x86/abi.rs

            }
        }
    }
 }

 /// Insert an epilogue given a specific `return` instruction.
 /// This is used by common calling conventions such as System V.
+/// TODO implement and handle _cfa_state more than one epilogue.


nit: outdated comment, can be removed I think?

cranelift-codegen/src/isa/x86/abi.rs

bnjbvr · 2019-10-24T16:38:11Z

cranelift-codegen/src/isa/x86/abi.rs

    if stack_size > 0 {
        pos.ins().adjust_sp_up_imm(Imm64::new(stack_size));
    }

    // Pop all the callee-saved registers, stepping backward each time to
    // preserve the correct order.
    let fp_ret = pos.ins().x86_pop(reg_type);
+    // account for CFA state in the reverse of `insert_common_prologue`.


nit: for all the comments here, please capitalize the first letter.

bnjbvr · 2019-10-24T16:39:26Z

cranelift-codegen/src/isa/x86/abi.rs

+                    .into_iter()
+                    .map(|x| *x)
+                    .chain(std::iter::once(new_cfa))
+                    .collect::<Box<[_]>>();


Any reason we don't use a vec! for the changes? We could then use push here.

#679 (comment) 😬

That was based on the fact that FrameLayoutChanges is a Box<[_]> and not a Vec<_>. Using Vec<_> for FrameLayoutChanges and then using pop would be faster. I didn't actually look why there was a Box<[_]> in the first place when writing that comment.

Ok, we could use a Vec here, or keep the Box. Not sure it's very performance-sensitive since it's tied to debug collection mode.

Not sure it's very performance-sensitive since it's tied to debug collection mode.

It would be perf sensitive for cg_clif once I add unwinding support based on this.

bnjbvr · 2019-10-24T16:41:40Z

cranelift-codegen/src/isa/x86/abi.rs

+    cfa_state.current_depth += word_size;
+    cfa_state.cf_ptr_offset -= word_size;
+    // and now that we're going to overwrite `rbp`, `rsp` is the only way to get to the call frame.
+    cfa_state.cf_ptr_reg = RU::rsp as RegUnit;


Don't we need to insert the layout changes after this instruction?

Great eye, https://github.com/CraneStation/cranelift/pull/679/files/f6582f0eafb1c9202b589944c59f4d8ffbb1c311#diff-08ef6b0c3d44ab300b5158dab40ea830R835 should be fp_ret.

iximeow · 2019-11-05T00:06:53Z

@yurydelendik will you have time to address the comments here or should I do so and PR into your fork again?

yurydelendik · 2019-11-05T23:22:24Z

@yurydelendik will you have time to address the comments here or should I do so and PR into your fork again?

@iximeow at this moment, I lost track on what is going on with this PR. It will take me some time to address the review comments. If you can address the comments, which will be awesome, I recommend to just submit your fork's branch as a new/different PR -- and I'll close this one.

iximeow · 2019-11-05T23:49:14Z

I'll open a new PR and address the remaining comments there. Thanks!

bnjbvr · 2019-11-06T11:07:38Z

Thanks both! I'll close this one since work has been continued in #1204.

yurydelendik force-pushed the frame-layout branch from 4bc31cf to 6e15d79 Compare February 19, 2019 22:15

yurydelendik mentioned this pull request Feb 19, 2019

Write .debug_frame information bytecodealliance/wasmtime#53

Merged

yurydelendik force-pushed the frame-layout branch from 6e15d79 to 022bc40 Compare February 21, 2019 14:17

yurydelendik force-pushed the frame-layout branch from 022bc40 to 2024f3b Compare March 7, 2019 13:42

bnjbvr requested a review from sunfishcode March 22, 2019 16:20

yurydelendik force-pushed the frame-layout branch from 2024f3b to 17251bd Compare April 5, 2019 13:04

philipc reviewed Apr 8, 2019

View reviewed changes

cranelift-codegen/src/ir/function.rs Outdated Show resolved Hide resolved

cranelift-codegen/src/ir/function.rs Outdated Show resolved Hide resolved

cranelift-codegen/src/isa/x86/abi.rs Show resolved Hide resolved

yurydelendik force-pushed the frame-layout branch 5 times, most recently from f1d8669 to 3fb342b Compare April 9, 2019 16:08

yurydelendik force-pushed the frame-layout branch from 3fb342b to 1c185ff Compare June 20, 2019 22:39

yurydelendik force-pushed the frame-layout branch from c61a15b to 49e5793 Compare July 3, 2019 15:06

bnjbvr reviewed Jul 8, 2019

View reviewed changes

yurydelendik force-pushed the frame-layout branch from 49e5793 to 10101b6 Compare July 9, 2019 19:24

philipc mentioned this pull request Aug 7, 2019

Stack unwinding improvements bytecodealliance/lucet#254

Open

iximeow mentioned this pull request Aug 15, 2019

FrameLayoutChange-based .eh_frame support #902

Closed

iximeow reviewed Aug 15, 2019

View reviewed changes

cranelift-codegen/src/isa/x86/abi.rs Show resolved Hide resolved

iximeow mentioned this pull request Sep 21, 2019

Track epilogue frame layout changes yurydelendik/cranelift#1

Merged

bjorn3 suggested changes Sep 24, 2019

View reviewed changes

jyn514 reviewed Sep 25, 2019

View reviewed changes

cranelift-codegen/src/ir/framelayout.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Oct 14, 2019

View reviewed changes

iximeow mentioned this pull request Oct 18, 2019

Implement emitting Windows unwind information for fastcall functions. #1155

Merged

yurydelendik force-pushed the frame-layout branch from e543dc3 to 80977a6 Compare October 22, 2019 18:23

yurydelendik and others added 4 commits October 22, 2019 13:26

Track frame layout changes.

9f77c9e

add epilogue cfa

ac4b27d

cleanup and avoid some unnecessary allocs

ac0a60e

add documentation for CFAState

f6582f0

yurydelendik force-pushed the frame-layout branch from 80977a6 to f6582f0 Compare October 22, 2019 18:27

bnjbvr self-requested a review October 23, 2019 07:52

bnjbvr suggested changes Oct 24, 2019

View reviewed changes

iximeow mentioned this pull request Nov 6, 2019

Track frame layout changes. (cont.d) #1204

Merged

4 tasks

bnjbvr closed this Nov 6, 2019

Track frame layout changes. #679

Track frame layout changes. #679

Conversation

yurydelendik commented Feb 19, 2019

bjorn3 commented Feb 21, 2019

lachlansneff commented Mar 11, 2019

philipc commented Apr 20, 2019

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr commented Jul 10, 2019

bjorn3 commented Jul 10, 2019 • edited Loading

sunfishcode commented Sep 7, 2019

iximeow commented Sep 24, 2019

iximeow commented Oct 14, 2019

Choose a reason for hiding this comment

bjorn3 commented Oct 14, 2019

iximeow commented Oct 14, 2019

bnjbvr commented Oct 21, 2019

bnjbvr commented Oct 22, 2019

yurydelendik commented Oct 22, 2019

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iximeow commented Nov 5, 2019

yurydelendik commented Nov 5, 2019

iximeow commented Nov 5, 2019

bnjbvr commented Nov 6, 2019

bjorn3 commented Jul 10, 2019 •

edited

Loading