Stream: git-cranelift

Topic: cranelift / PR #1320 Refactor unwind; add FDE support.

GitHub (Jan 07 2020 at 21:02):

yurydelendik opened PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [ ] use feature to cfg existing unwind logic
• [ ] agree on location for FDE/UNWIND_INFO logic
• [ ] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 07 2020 at 22:34):

bjorn3 submitted PR Review.

GitHub (Jan 07 2020 at 22:34):

bjorn3 submitted PR Review.

GitHub (Jan 07 2020 at 22:34):

bjorn3 created PR Review Comment:

On macOS DWARF relocations need to be section relative and not symbol relative.

GitHub (Jan 07 2020 at 22:34):

bjorn3 created PR Review Comment:

Leftover?

GitHub (Jan 07 2020 at 22:34):

bjorn3 created PR Review Comment:

This only works when compiling for one isa during the full process execution and is not thread safe.

GitHub (Jan 07 2020 at 22:35):

bjorn3 created PR Review Comment:

Maybe Seh and Dwarf?

GitHub (Jan 07 2020 at 22:35):

bjorn3 submitted PR Review.

GitHub (Jan 07 2020 at 22:35):

bjorn3 submitted PR Review.

GitHub (Jan 07 2020 at 22:44):

yurydelendik submitted PR Review.

GitHub (Jan 07 2020 at 22:44):

yurydelendik created PR Review Comment:

Yes. Currently I'm just looking feedback on API design -- I'll tie ends with FDE support in a day or two.

GitHub (Jan 07 2020 at 22:58):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [ ] use feature to cfg existing unwind logic
• [ ] agree on location for FDE/UNWIND_INFO logic
• [ ] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 07 2020 at 22:59):

yurydelendik submitted PR Review.

GitHub (Jan 07 2020 at 22:59):

yurydelendik created PR Review Comment:

Not sure if it is applied here: the FDE entry is for __register_frame

GitHub (Jan 07 2020 at 23:02):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [ ] use feature to cfg existing unwind logic
• [ ] agree on location for FDE/UNWIND_INFO logic
• [ ] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 07 2020 at 23:02):

yurydelendik submitted PR Review.

GitHub (Jan 07 2020 at 23:02):

yurydelendik created PR Review Comment:

Changed to Libunwind

GitHub (Jan 07 2020 at 23:53):

peterhuene submitted PR Review.

GitHub (Jan 07 2020 at 23:53):

peterhuene submitted PR Review.

GitHub (Jan 07 2020 at 23:53):

peterhuene created PR Review Comment:

Can we derive the kind from the TargetIsa (i.e. internal to its emit_unwind_info implementation) so it doesn't need to be passed in? I don't particularly see a need for requesting a different kind of unwind info that doesn't match the ISA's calling convention.

GitHub (Jan 07 2020 at 23:53):

peterhuene created PR Review Comment:

I think we'll want to pass the sink on down into UnwindInfo::emit so we're not double buffering these bytes (once here and then presumably in the caller's sink impl).

GitHub (Jan 07 2020 at 23:53):

peterhuene edited PR Review Comment.

GitHub (Jan 07 2020 at 23:55):

peterhuene created PR Review Comment:

Also, I'm totally open to renaming UnwindInfo so it's more specific to Windows so this gets less confusing.

GitHub (Jan 07 2020 at 23:55):

peterhuene submitted PR Review.

GitHub (Jan 08 2020 at 19:17):

yurydelendik submitted PR Review.

GitHub (Jan 08 2020 at 19:17):

yurydelendik created PR Review Comment:

Right, but now I need to specify in which format the data was produced. The caller now have to react on type of data it receives.

GitHub (Jan 08 2020 at 19:17):

yurydelendik edited PR Review Comment.

GitHub (Jan 08 2020 at 19:18):

yurydelendik submitted PR Review.

GitHub (Jan 08 2020 at 19:18):

yurydelendik created PR Review Comment:

Not an issue atm.

GitHub (Jan 08 2020 at 19:18):

yurydelendik edited PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [ ] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [ ] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 08 2020 at 20:10):

peterhuene submitted PR Review.

GitHub (Jan 08 2020 at 20:10):

peterhuene created PR Review Comment:

I see. Are you planning on having a sink implementation that differs based on the unwind kind then?

GitHub (Jan 08 2020 at 20:28):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [ ] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [ ] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 08 2020 at 20:28):

yurydelendik edited PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [ ] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [x] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 08 2020 at 20:30):

yurydelendik created PR Review Comment:

That's depend on the consumer, but in case of wasmtime, I planned to have one.

GitHub (Jan 08 2020 at 20:30):

yurydelendik submitted PR Review.

GitHub (Jan 08 2020 at 20:30):

yurydelendik edited PR Review Comment.

GitHub (Jan 08 2020 at 20:33):

peterhuene created PR Review Comment:

:+1:

GitHub (Jan 08 2020 at 20:33):

peterhuene submitted PR Review.

GitHub (Jan 08 2020 at 21:26):

bjorn3 submitted PR Review.

GitHub (Jan 08 2020 at 21:26):

bjorn3 submitted PR Review.

GitHub (Jan 08 2020 at 21:26):

bjorn3 created PR Review Comment:

Please check that the isa is x86_64 for both map_reg and RETURN_ADDRESS_REGISTER.

GitHub (Jan 08 2020 at 21:53):

yurydelendik created PR Review Comment:

okay, I probably just need to implement that for X86 too.

GitHub (Jan 08 2020 at 21:53):

yurydelendik submitted PR Review.

GitHub (Jan 08 2020 at 22:34):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [ ] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [x] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 08 2020 at 22:40):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [ ] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [x] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 08 2020 at 22:41):

yurydelendik edited PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [x] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [x] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 09 2020 at 09:14):

philipc submitted PR Review.

GitHub (Jan 09 2020 at 09:14):

philipc created PR Review Comment:

You should change this to only enable the "write" feature (also see https://github.com/gimli-rs/gimli/pull/466 which reduces the number of dependencies for this).

gimli = { version = "0.19.0", default-features = false, features = ["write"], optional = true }

GitHub (Jan 09 2020 at 15:36):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [x] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [x] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 09 2020 at 21:49):

peterhuene submitted PR Review.

GitHub (Jan 09 2020 at 23:25):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [x] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [x] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 10 2020 at 15:20):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [x] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [x] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 10 2020 at 22:10):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [x] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [x] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 10 2020 at 22:12):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [x] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [x] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 10 2020 at 22:52):

yurydelendik updated PR #1320 from unwind-fde to master:

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 https://github.com/bytecodealliance/wasmtime/pull/759

TODO (and requires feedback):

• [x] use feature to cfg existing unwind logic
• [x] agree on location for FDE/UNWIND_INFO logic
• [x] borrow reg mapper from #902

cc @peterhuene @iximeow

GitHub (Jan 13 2020 at 16:32):

yurydelendik merged PR #1320.

GitHub (Jan 13 2020 at 20:19):

iximeow submitted PR Review.

GitHub (Jan 13 2020 at 20:19):

iximeow created PR Review Comment:

I don't think 8-byte absolute relocations are necessarily available for all targets - specifically, as far as I know, MachO objects only permit up to dword-size relocations. This is one of the reasons I used the address encoding SDATA4 in #902. Are qword relocations available in MachO targets? If so, I also prefer absolute relocations, but I'd like to not exclude macos here.

GitHub (Jan 13 2020 at 20:22):

iximeow submitted PR Review.

GitHub (Jan 13 2020 at 20:22):

iximeow created PR Review Comment:

checking that pointer_bits() is 64 is a good call, I'd neglected to think about 32-bit targets. That said, I'll put up a PR to extend these to support i686 register numbers (gimli already has constants for them!).

With 32-bit register numbers, the assert would just be assert!(isa.name() == "x86"), which seems redundant given that this is in isa/x86/fde.rs.

GitHub (Jan 13 2020 at 20:26):

iximeow submitted PR Review.

GitHub (Jan 13 2020 at 20:26):

iximeow created PR Review Comment:

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.

GitHub (Jan 13 2020 at 20:29):

iximeow submitted PR Review.

GitHub (Jan 13 2020 at 20:29):

iximeow created PR Review Comment:

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.

GitHub (Jan 13 2020 at 20:31):

iximeow created PR Review Comment:

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

GitHub (Jan 13 2020 at 20:31):

iximeow submitted PR Review.

GitHub (Jan 13 2020 at 20:32):

iximeow submitted PR Review.

GitHub (Jan 13 2020 at 20:32):

iximeow created PR Review Comment:

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

GitHub (Jan 13 2020 at 20:33):

iximeow submitted PR Review.

GitHub (Jan 13 2020 at 20:33):

iximeow created PR Review Comment:

:+1:! I'd not thought about basic blocks possibly being out of order in the layout.

GitHub (Jan 13 2020 at 20:35):

iximeow submitted PR Review.

GitHub (Jan 13 2020 at 20:35):

iximeow created PR Review Comment:

:+1: :+1:! I was extremely unsure how to test that #902 generated expected unwind information, other than checking that Lucet could unwind correctly with those patches. This is great, thank you.

GitHub (Jan 13 2020 at 20:40):

iximeow submitted PR Review.

GitHub (Jan 13 2020 at 20:40):

iximeow created PR Review Comment:

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?

GitHub (Jan 13 2020 at 21:03):

yurydelendik submitted PR Review.

GitHub (Jan 13 2020 at 21:03):

yurydelendik created PR Review Comment:

This serialization targets specific use case of libunwind (in context of JIT), and it requires FDE in specific format. This PR will not address other use cases.

GitHub (Jan 13 2020 at 21:05):

yurydelendik submitted PR Review.

GitHub (Jan 13 2020 at 21:05):

yurydelendik created PR Review Comment:

the assert would just be assert!(isa.name() == "x86"), which seems redundant given that this is in isa/x86/fde.rs.

yes, I just wanted to address the reviewer's comment to complete satisfaction and note that function is indeed x86 specific.

GitHub (Jan 13 2020 at 21:07):

yurydelendik submitted PR Review.

GitHub (Jan 13 2020 at 21:07):

yurydelendik created PR Review Comment:

yep, that's a bug

GitHub (Jan 13 2020 at 21:17):

iximeow submitted PR Review.

GitHub (Jan 13 2020 at 21:17):

iximeow created PR Review Comment:

good to know, thank you - I'll be revising #902 to support writing .eh_frame to object files, and I'll make sure to preserve this relocation style.

GitHub (Jan 14 2020 at 22:26):

philipc submitted PR Review.

GitHub (Jan 14 2020 at 22:26):

philipc created PR Review Comment:

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

GitHub (Jan 14 2020 at 22:31):

philipc submitted PR Review.

GitHub (Jan 14 2020 at 22:31):

philipc created PR Review Comment:

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?

Last updated: Nov 22 2024 at 16:03 UTC