Refactor unwind; add FDE support. #1320

yurydelendik · 2020-01-07T21:02:32Z

Currently, codegen produces unwind information only for Windows platform. We need to produce this type of information for all platforms to properly produce backtrace. See bytecodealliance/wasmtime#759

TODO (and requires feedback):

use feature to cfg existing unwind logic
agree on location for FDE/UNWIND_INFO logic
borrow reg mapper from FrameLayoutChange-based .eh_frame support #902

cc @peterhuene @iximeow

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

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

cranelift-codegen/src/binemit/mod.rs

cranelift-codegen/src/context.rs

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

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

cranelift-codegen/Cargo.toml

peterhuene

Looks good!

I think some tests for fde.rs might be warranted to test the FDE data emission.

I also think a follow-up PR to make test_unwind.rs more generic so that it supports FDE generation in addition to Windows unwind information would be ideal. That would enable us to author some clif-based tests for checking the FDE data for non-fastcall functions.

yurydelendik · 2020-01-10T22:16:37Z

I also think a follow-up PR to make test_unwind.rs more generic so that it supports FDE generation in addition to Windows unwind information would be ideal.

Yeah, currently I added "test_fde.rs" -- it will match how wasmtime will use the functionality, by using FrameUnwindKind parameter.

peterhuene · 2020-01-10T22:23:16Z

👍

I think in a future PR we might want to unify the unwind and fde subtests so that it'll print out the right thing depending on the given function's ABI and calling convention.

I feel that unwind is too generic a term to mean just Windows unwind information, so it might lead to confusion in the long run. Perhaps something to revisit when we further abstract things.

Co-Authored-By: Philip Craig <philipjcraig@gmail.com>

iximeow · 2020-01-13T20:17:48Z

sorry for the late comments - I was out last week and as a result wasn't checking cranelift too closely. I do have a few comments, but I expect them to be subsequent PRs, and would like to discuss a bit first.

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

iximeow · 2020-01-13T20:26:49Z

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

@@ -0,0 +1,354 @@
+//! Support for FDE data generation.


other than the register mapping, and possibly code/data alignment, FDE generation is architecture-agnostic. I assume you rather this code in cranelift-codegen for use in wasmtime, but I'd like to have a very clear path to supporting other architectures. Is there another reason this would be in isa/x86/? If not I'd like to move it to somewhere like cranelift-codegen/src/binemit/unwind.rs that is more clearly independent of a specific architecture.

iximeow · 2020-01-13T20:29:01Z

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

+    }
+}
+
+fn return_address_reg(isa: &dyn TargetIsa) -> Register {


Also on the point of being architecture agnostic, I'd specifically wanted DwarfRegMapper to clearly indicate how to add support for non-x86_64 architectures. You mentioned wanting cranelift-module changed for other reasons, is there something unsuitable about struct DwarfRegMapper in #902? If it was sufficient, and we're just deferring non-x86 thought for the moment, I'd like to revive DwarfRegMapper in a subsequent PR.

iximeow · 2020-01-13T20:31:10Z

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

+fn to_cfi(
+    isa: &dyn TargetIsa,
+    change: &FrameLayoutChange,
+    cfa_def_reg: &mut Register,


I strongly prefer isa::RegUnit here for debugging reasons - going backwards from Register to isa::RegUnit. Does something necessitate this being gimli::Register?

iximeow · 2020-01-13T20:32:26Z

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

+        FrameLayoutChange::ReturnAddressAt { cfa_offset } => {
+            assert!(cfa_offset % -8 == 0);
+            let cfa_offset = *cfa_offset as i32;
+            CallFrameInstruction::Offset(X86_64::RA, cfa_offset)


quibble: if X86_64::RA were replaced with return_address_reg(isa), to_cfi would not have any x86-specific details anymore

yep, that's a bug

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

filetests/isa/x86/windows_systemv_x64_fde.clif

iximeow · 2020-01-13T20:40:26Z

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

+    for (off, r) in relocs {
+        sink.reloc(r, off + unwind_start);
+    }
+    let fde_offset = unsafe { ptr::read::<u32>(bytes.as_ptr() as *const u32) } as usize + 4;


I'm not sure if this just snuck by in review or not, but I'd at the very least want an explanation next to this pointer read with why it will be safe. I'd like to replace this with u32::from_le_bytes(bytes.as_slice()[..8].try_into()) - since the checks here aren't in a particularly hot path it seems like that shouldn't be an issue?

This whole thing is fighting against the gimli API a bit. You're using FrameTable to write a single CIE and FDE, and then trying to figure out the FDE offset.

How about if gimli made the CIE/FDE write methods public, and then you can call those methods instead of using FrameTable, and get the FDE offset simply as bytes.len() prior to calling the FDE write?

yurydelendik · 2020-01-13T21:12:17Z

I do have a few comments, but I expect them to be subsequent PRs

@iximeow thank you for review, I'll try to get back to them shortly after bytecodealliance/wasmtime#759 . As @peterhuene noted, there will be more refactoring connected with generalizing unwind information.

philipc · 2020-01-14T22:26:41Z

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

+    let mut eh_frame = EhFrame::from(FDEWriter::new());
+    frames.write_eh_frame(&mut eh_frame).unwrap();
+
+    let (bytes, relocs) = eh_frame.clone().into_vec_and_relocs();


Use eh_frame.0 instead of eh_frame.clone().

bnjbvr · 2020-01-15T09:17:38Z

Random drive-by comment for a future PR: can you explicit what the acronym FDE means, at the top of the fde.rs file, for instance?

bjorn3 suggested changes Jan 7, 2020

View reviewed changes

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

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

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

bjorn3 reviewed Jan 7, 2020

View reviewed changes

cranelift-codegen/src/binemit/mod.rs Outdated Show resolved Hide resolved

yurydelendik force-pushed the unwind-fde branch 2 times, most recently from 29b824e to 58046ee Compare January 7, 2020 23:02

peterhuene reviewed Jan 7, 2020

View reviewed changes

cranelift-codegen/src/context.rs Show resolved Hide resolved

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

yurydelendik marked this pull request as ready for review January 8, 2020 20:28

bjorn3 reviewed Jan 8, 2020

View reviewed changes

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

yurydelendik force-pushed the unwind-fde branch from 038bb20 to dd0f8b0 Compare January 8, 2020 22:34

philipc reviewed Jan 9, 2020

View reviewed changes

cranelift-codegen/Cargo.toml Outdated Show resolved Hide resolved

peterhuene approved these changes Jan 9, 2020

View reviewed changes

yurydelendik force-pushed the unwind-fde branch 2 times, most recently from 4a70f72 to fc09d34 Compare January 10, 2020 22:12

yurydelendik and others added 7 commits January 10, 2020 16:50

Refactor unwind; add FDE support.

e790776

use sink directly in emit functions

511a1ae

Better X86 registers mapping for FDE.

4e231cf

pref off all unwinding generation with feature

243b04d

only enable the "write" feature for gimli

1f03121

Co-Authored-By: Philip Craig <philipjcraig@gmail.com>

make FrameUnwindKind clonable and eq-able; fix func length

3ac7c8d

Add FDE tests.

7cfc4c9

yurydelendik force-pushed the unwind-fde branch from fc09d34 to 7cfc4c9 Compare January 10, 2020 22:52

yurydelendik merged commit e6e6715 into bytecodealliance:master Jan 13, 2020

yurydelendik mentioned this pull request Jan 13, 2020

Backtrace WebAssembly function JIT frames bytecodealliance/wasmtime#759

Merged

7 tasks

iximeow reviewed Jan 13, 2020

View reviewed changes

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

iximeow reviewed Jan 13, 2020

View reviewed changes

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

iximeow reviewed Jan 13, 2020

View reviewed changes

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

iximeow reviewed Jan 13, 2020

View reviewed changes

filetests/isa/x86/windows_systemv_x64_fde.clif Show resolved Hide resolved

iximeow reviewed Jan 13, 2020

View reviewed changes

philipc reviewed Jan 14, 2020

View reviewed changes

yurydelendik mentioned this pull request Jan 16, 2020

Fix remaining clippy warnings #1340

Merged

4 tasks

iximeow mentioned this pull request Feb 28, 2020

Improve support for bulk function unwind information bytecodealliance/wasmtime#1181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor unwind; add FDE support. #1320

Refactor unwind; add FDE support. #1320

yurydelendik commented Jan 7, 2020 •

edited

Loading

peterhuene left a comment

yurydelendik commented Jan 10, 2020

peterhuene commented Jan 10, 2020 •

edited

Loading

iximeow commented Jan 13, 2020

iximeow Jan 13, 2020

iximeow Jan 13, 2020

iximeow Jan 13, 2020

iximeow Jan 13, 2020

yurydelendik Jan 13, 2020

iximeow Jan 13, 2020

philipc Jan 14, 2020

yurydelendik commented Jan 13, 2020

philipc Jan 14, 2020

bnjbvr commented Jan 15, 2020

Refactor unwind; add FDE support. #1320

Refactor unwind; add FDE support. #1320

Conversation

yurydelendik commented Jan 7, 2020 • edited Loading

peterhuene left a comment

Choose a reason for hiding this comment

yurydelendik commented Jan 10, 2020

peterhuene commented Jan 10, 2020 • edited Loading

iximeow commented Jan 13, 2020

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

yurydelendik commented Jan 13, 2020

Choose a reason for hiding this comment

bnjbvr commented Jan 15, 2020

yurydelendik commented Jan 7, 2020 •

edited

Loading

peterhuene commented Jan 10, 2020 •

edited

Loading