FrameLayoutChange-based .eh_frame support #902

iximeow · 2019-08-15T21:36:04Z

This teaches cranelift-faerie how to write .eh_frame sections that are appropriate with respect to call frames for functions that produce layout information.

This builds quite directly on #679, which is still open, and whose changes are alongside this as a result. For the time being, my changes on top of @yurydelendik 's PR are best seen as a diff between our branches.

Being dependent on an already-floating PR, I want to mostly call attention to the points that:

frame change information is useful for handling unwinding as @bjorn3 mentioned here
consumers of Cranelift (in this case, Lucet) probably want to unwind through Cranelift-generated code
Track frame layout changes. #679 is good enough to build on to the above ends, and the changes to produce .eh_frame and support unwinding through Cranelift-generated code are fairly small.

I do have some comments in-line in case any readers do take a closer look at the changes here, but I'm not sure it makes sense to sweat the details until #679 is figured out, since it seems like we've bitrotted with respect to today's master anyway.

cranelift-faerie/src/backend.rs

iximeow · 2019-08-15T21:41:22Z

cranelift-faerie/src/backend.rs

+    reg_info: RegInfo,
+}
+
+impl<'a> DwarfRegMapper<'a> {


I mentioned this on irc a few days ago but DwarfRegMapper could arguably live in cranelift-codegen as it's fundamentally an int->int function, and doesn't need to return a gimli::Register. This and CFIEncoder might also make sense in cranelift-module as they are, pulling in a gimli dependency there and not in cranelift-codegen. @yurydelendik would cranelift-module be a useful place to share this logic translating FrameLocationChange to CFI instructions for your use in wasmtime?

Issue #729 is about the register mapping.

I've moved this to cranelift-module and noted some reasoning in that issue. I'll leave this unresolved until we decide here or there what to do with DwarfRegMapper.

@yurydelendik would cranelift-module be a useful place to share this logic translating FrameLocationChange to CFI instructions for your use in wasmtime?

@iximeow yes, and in fact, I would like to have entire cratelift-module change for other reasons.

philipc · 2019-08-16T03:54:08Z

cranelift-faerie/src/backend.rs

+            gimli::Encoding {
+                format: gimli::Format::Dwarf32,
+                version: 1,
+                address_size: 4,


This probably should be 8. Not sure if it will be used for the data you are writing though.

It should only be 8 on x86_64.

I had definitely misunderstood the point of the address_size field here to be "size of an encoded pointer", rather than "size of a pointer for the target ISA". I'll make this pick the right size for the target.

4 and the asserts later about address size/pointer encoding, are a relic of trying to match CIE bytes I'd known work, in tracking down a crash in libgcc. In reality, I eventually figured out the issue I'd had is that libgcc doesn't handle escaped length fields in 64-bit DWARF, so I just tried absptr encodings and it still works fine! 🎉

I ought to add a note about why this specifically uses Dwarf32 regardless of architecture - in other places I at least wanted to at least comment about architecture-specific magic values, so format deserves a Do not change this! mention.

Thinking through my confusion with the meaning of this and the write_eh_pointer argument, it seems like write_eh_pointer ought to not take address_size as a parameter. Since it only matters for absolute pointer encodings, I think it could work better to provide address_size as a field of an absptr variant for DwEhPe? Then it would be clear just from the function signature that the implementer is to write an address matching the encoding, and that gimli is not making assumptions about the number of bytes that would be written via address_size. If that sounds interesting, I can sketch out a PR to gimli to try that.

The Encoding::address_size field is used for all DWARF, where the normal format of an address is an unencoded pointer. .eh_frame is different from DWARF in that it encodes pointers, but even then it can't be the encoded pointer size because it is possible to use multiple encodings of different sizes.

DwEhPe is used to represent the constants, so it can't be changed. The size parameter to write_eh_pointer is documented as only being used for absolute pointers. I'm not sure we can do better than that. Maybe renaming the parameter to absolute_address_size would be worthwhile.

We may want to add a check in gimli to ensure you don't write .eh_frame with Dwarf64, since the LSB requires 32-bit length fields.

cranelift-faerie/src/backend.rs

bjorn3

🎉

Next step will be running user code during cleanup.

cranelift-faerie/src/backend.rs

bjorn3 · 2019-08-16T18:09:47Z

cranelift-faerie/src/backend.rs

+                    .link_with(
+                        faerie::Link {
+                            to: name,
+                            from: ".eh_frame",


Is this __eh_frame for machO?

That's a good question, I'll at least see what other toolchains name it. I also need to use a machO-appropriate relocation.

cranelift-faerie/src/backend.rs

bjorn3 · 2019-08-16T18:29:29Z

cranelift-faerie/src/backend.rs

+
+                frame_sink.add_fde(cie, fd_entry);
+            } else {
+                // we have a frame sink to write .eh_frames into, but are not collecting debug


Please panic here to make it easier to debug in that case.

I realized this morning that there's a case where we will reach this arm but not be a bug: if the user of cranelift has a per-function nounwind-like attribute, a nounwind function wouldn't need unwind information collected, even though there still would be a frame sink for other functions. Right now it happens to be that Lucet is all-or-nothing on unwind info, but as a rustc backend for example I could see some function being opt-out.

Given that, would you rather a panic to identify the case for later discussion, or should I discard the else entirely?

but as a rustc backend for example I could see some function being opt-out.

I believe unwinding enabled/disabled is done per crate (using -Cpanic=abort), not per function.

rustc has the unstable #[unwind(allowed/aborts)] to control per function, as well I think it has different requirements for extern "C", but I don't know the details of that and it is under ongoing discussion.

philipc · 2019-11-26T03:35:43Z

cranelift-module/src/dwarf.rs

+    // we need to retain function names to hand out usize identifiers for FDE addresses,
+    // which are then used to look up function names again for relocations, when `write_address` is
+    // called.
+    fn_names: Vec<String>,


ModuleContents does already contain something like this, not sure if it is worth trying to reuse that, but maybe FuncId::as_u32 can be the identifier.

some x86-ism magic numbers are still present, but they at least should be suboptimal (rather than invalid) for non-x86 architectures.

…odule

alexcrichton · 2020-02-28T23:35:36Z

Thanks for the PR again, and as a procedural note the Cranelift repository has now merged into the wasmtime repository.

PRs are no longer landing in this repository, and unfortunately there's no "one button" solution to move a PR to the wasmtime repository. A script has been prepared, however, to assist you in transferring this PR to the wasmtime repo. Feel free to reach out on Zulip with any questions!

iximeow · 2020-03-03T21:26:28Z

Closing this with the hope that either #1181 will result in a change that supplants PR, or that this will be significantly different when #1181 is resolved.

iximeow commented Aug 15, 2019

View reviewed changes

cranelift-faerie/src/backend.rs Outdated Show resolved Hide resolved

iximeow commented Aug 15, 2019

View reviewed changes

cranelift-faerie/src/backend.rs Outdated Show resolved Hide resolved

iximeow commented Aug 15, 2019

View reviewed changes

philipc reviewed Aug 16, 2019

View reviewed changes

bjorn3 reviewed Aug 16, 2019

View reviewed changes

cranelift-faerie/src/backend.rs Outdated Show resolved Hide resolved

bjorn3 suggested changes Aug 16, 2019

View reviewed changes

bjorn3 reviewed Aug 16, 2019

View reviewed changes

This was referenced Sep 3, 2019

WIP: "C panic" ABI specifier rust-lang/rfcs#2753

Closed

Stack unwinding improvements bytecodealliance/lucet#254

Open

This was referenced Oct 14, 2019

Track frame layout changes. #679

Closed

Implement emitting Windows unwind information for fastcall functions. #1155

Merged

iximeow mentioned this pull request Nov 6, 2019

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

Merged

4 tasks

iximeow force-pushed the framechange_eh_frame_support branch from ac21f47 to 9ac709e Compare November 25, 2019 23:18

philipc reviewed Nov 26, 2019

View reviewed changes

philipc mentioned this pull request Dec 18, 2019

Emit debug info jyn514/saltwater#152

Open

yurydelendik mentioned this pull request Jan 7, 2020

Refactor unwind; add FDE support. #1320

Merged

3 tasks

iximeow added 6 commits January 16, 2020 14:45

initial commit adding .eh_frame creation for x86_64 in cranelift-faerie

33784f2

build .eh_frame info via gimli

88d6c2c

decouple eh_frame creation from x86 magic numbers a bit

dc3dc2e

some x86-ism magic numbers are still present, but they at least should be suboptimal (rather than invalid) for non-x86 architectures.

first part of review comments

c865e87

move non-faerie-specific parts of .eh_frame generation to cranelift-m…

5e586cd

…odule

remove wrong write_address impl

b1c81fd

This was referenced Jan 16, 2020

[module] Finalize definitions for the end-user #1290

Merged

de-public fn finalize_definitions #1353

Closed

iximeow force-pushed the framechange_eh_frame_support branch from 73e2a03 to b1c81fd Compare January 18, 2020 01:20

iximeow mentioned this pull request Feb 28, 2020

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

Closed

alexcrichton mentioned this pull request Mar 3, 2020

Windows (Fastcall) calling convention: Callee-saved XMM (FP) registers are not actually saved bytecodealliance/wasmtime#1177

Closed

iximeow closed this Mar 3, 2020

iximeow mentioned this pull request Mar 11, 2020

Add capability (to cranelift) to generate .eh_frame bytecodealliance/wasmtime#1282

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FrameLayoutChange-based .eh_frame support #902

FrameLayoutChange-based .eh_frame support #902

iximeow commented Aug 15, 2019 •

edited

Loading

iximeow Aug 15, 2019

philipc Aug 16, 2019

iximeow Nov 25, 2019

yurydelendik Dec 19, 2019

philipc Aug 16, 2019

bjorn3 Aug 16, 2019

iximeow Aug 16, 2019

philipc Aug 16, 2019

bjorn3 left a comment

bjorn3 Aug 16, 2019

iximeow Aug 16, 2019

bjorn3 Aug 16, 2019

iximeow Aug 16, 2019

bjorn3 Aug 16, 2019

philipc Nov 26, 2019

philipc Nov 26, 2019

alexcrichton commented Feb 28, 2020

iximeow commented Mar 3, 2020

FrameLayoutChange-based .eh_frame support #902

FrameLayoutChange-based .eh_frame support #902

Conversation

iximeow commented Aug 15, 2019 • edited Loading

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

bjorn3 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

alexcrichton commented Feb 28, 2020

iximeow commented Mar 3, 2020

iximeow commented Aug 15, 2019 •

edited

Loading