wasmtime / PR #1466 Refactor unwind generation in Cranelift. · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1466 Refactor unwind generation in Cranelift.

Wasmtime GitHub notifications bot (Apr 03 2020 at 20:55):

peterhuene opened PR #1466 from fix-unwind-emit to master:

This commit makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 03 2020 at 20:55):

peterhuene edited PR #1466 from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 03 2020 at 20:56):

peterhuene requested yurydelendik for a review on PR #1466.

Wasmtime GitHub notifications bot (Apr 03 2020 at 21:09):

peterhuene updated PR #1466 from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 03 2020 at 21:48):

peterhuene updated PR #1466 from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 03 2020 at 22:26):

peterhuene updated PR #1466 from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 03 2020 at 22:41):

peterhuene updated PR #1466 from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 08 2020 at 18:39):

peterhuene edited PR #1466 from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention in Wasmtime, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 08 2020 at 20:30):

abrown created PR Review Comment:

I think this might suffer from the same problem of #1471: when compiling for ARM, this block will be ignored and map_reg won't exist over in wasmtime-environ and wasmtime-debug. I guess we have the same options I mentioned in that issue:

to just get the ARM building, I could remove the feature flag on x86; then users encounter a runtime error if they try to use this FDE functionality but at least it compiles

to make map_reg functional on other platforms, we could either move map_reg out of the x86 module or create separate map_reg functions in all of the ISA modules.

Wasmtime GitHub notifications bot (Apr 08 2020 at 20:30):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2020 at 20:39):

peterhuene created PR Review Comment:

Ah, I missed that it was in wasmtime-environ and not in the cranelift build. I like the second solution, perhaps with a default implementation that errors with a new RegisterMappingError indicating the ISA doesn't support gimli register mapping?

Wasmtime GitHub notifications bot (Apr 08 2020 at 20:39):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2020 at 20:40):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 08 2020 at 20:41):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2020 at 20:41):

peterhuene created PR Review Comment:

Going to resolve this and take the conversation back to #1471.

Wasmtime GitHub notifications bot (Apr 13 2020 at 22:56):

peterhuene updated PR #1466 from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention in Wasmtime, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 14 2020 at 21:14):

peterhuene assigned PR #1466 to yurydelendik.

Wasmtime GitHub notifications bot (Apr 14 2020 at 21:14):

peterhuene requested bnjbvr and yurydelendik for a review on PR #1466.

Wasmtime GitHub notifications bot (Apr 15 2020 at 16:06):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 16:06):

yurydelendik created PR Review Comment:

Here and near map_reg in this file, we need to avoid panic. It maybe reasonable to just skip the operation if register is unknown to DWARF (I don't think we can create a fallback CallFrameInstruction).

Wasmtime GitHub notifications bot (Apr 15 2020 at 19:14):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 19:14):

peterhuene created PR Review Comment:

Good point. We could also return an error instead of panicking since the creation function now returns CodegenResult from #1216.

Wasmtime GitHub notifications bot (Apr 15 2020 at 19:54):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 19:54):

abrown created PR Review Comment:

map_reg should now return a Result since #1451 so it should be a bit easier; prior to that PR map_reg panicked so when I added RegisterMappingError I kept the same behavior with this expect (and one other place I believe). I agree it would be better if we avoided panicking!

Wasmtime GitHub notifications bot (Apr 15 2020 at 20:17):

peterhuene created PR Review Comment:

I missed that will all the merging. I'll get that fixed.

Wasmtime GitHub notifications bot (Apr 15 2020 at 20:17):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 20:43):

peterhuene deleted PR Review Comment.

Wasmtime GitHub notifications bot (Apr 15 2020 at 21:03):

peterhuene updated PR #1466 (assigned to yurydelendik) from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention in Wasmtime, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 15 2020 at 21:03):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 21:03):

peterhuene created PR Review Comment:

I've pushed up a commit to remove the expect from map_reg calls.

Wasmtime GitHub notifications bot (Apr 15 2020 at 21:50):

peterhuene updated PR #1466 (assigned to yurydelendik) from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention in Wasmtime, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 15 2020 at 21:59):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 21:59):

yurydelendik created PR Review Comment:

Thanks. FWIW I think generating even incomplete FDE for backtrace outweighs maintainability/diagnostics of unwind generation code. That said, we may ignore map_reg errors in future.

Wasmtime GitHub notifications bot (Apr 15 2020 at 22:53):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 22:53):

yurydelendik created PR Review Comment:

IIRC that needs to happen after each FDE: GCC __register_frame walks until the end (if my understanding of source code right), though on MacOS everything goes through _unw_add_dynamic_fde (single FDE only).

Wasmtime GitHub notifications bot (Apr 15 2020 at 22:54):

yurydelendik edited PR Review Comment.

Wasmtime GitHub notifications bot (Apr 15 2020 at 23:01):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 23:33):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 15 2020 at 23:33):

peterhuene created PR Review Comment:

Ugh, you're right. I think this is repeatedly going through the remaining part of the table each iteration to register the frame. I'll fix.

Wasmtime GitHub notifications bot (Apr 16 2020 at 00:39):

peterhuene updated PR #1466 (assigned to yurydelendik) from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention in Wasmtime, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 16 2020 at 04:34):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 04:34):

peterhuene created PR Review Comment:

So I put up a commit that calls __register_frame once for non-macOS and for macOS it iterates the table and calls __register_frame per FDE. Judging from the libgcc source, this should work just fine. Let me know what you think and I'll make any additional changes.

Wasmtime GitHub notifications bot (Apr 16 2020 at 09:12):

bnjbvr requested bnjbvr and yurydelendik for a review on PR #1466.

Wasmtime GitHub notifications bot (Apr 16 2020 at 14:02):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 18:15):

peterhuene updated PR #1466 (assigned to yurydelendik) from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention in Wasmtime, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 16 2020 at 19:14):

peterhuene updated PR #1466 (assigned to yurydelendik) from fix-unwind-emit to master:

This PR makes the following changes to unwind information generation in
Cranelift:

Remove frame layout change implementation in favor of processing the prologue
and epilogue instructions when unwind information is requested. This also
means this work is no longer performed for Windows, which didn't utilize it.
It also helps simplify the prologue and epilogue generation code.

Remove the unwind sink implementation that required each unwind information
to be represented in final form. For FDEs, this meant writing a
complete frame table per function, which wastes 20 bytes or so for each
function with duplicate CIEs. This also enables Cranelift users to collect the
unwind information and write it as a single frame table.

For System V calling convention in Wasmtime, the unwind information is no longer stored
in code memory (it's only a requirement for Windows ABI to do so). This allows
for more compact code memory for modules with a lot of functions.

Deletes some duplicate code relating to frame table generation. Users can
now simply use gimli to create a frame table from each function's unwind
information.

Fixes #1181.

Wasmtime GitHub notifications bot (Apr 16 2020 at 20:34):

peterhuene merged PR #1466.

Wasmtime GitHub notifications bot (Apr 28 2020 at 05:29):

philipc submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 05:29):

philipc created PR Review Comment:

FYI, gimli::write::Expression is no longer a simple Vec<u8> after https://github.com/gimli-rs/gimli/pull/479, so this isn't going to work.

It seems like you don't actually need impl From<gimli::write::CallFrameInstruction> for CallFrameInstruction though, so it would be possible to simply delete all the Expression support. If you do need to support expressions, then you'll need to come up with your own serializable version of them. gimli's expressions are used for other debug sections too, thus they potentially have references to DIEs and can't be individually serialized.

Wasmtime GitHub notifications bot (Apr 28 2020 at 06:03):

philipc submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 06:03):

philipc created PR Review Comment:

See https://github.com/philipc/wasmtime/commit/103d68d6c5e6201f76a17b14b29ddca82ad02350 for the changes I think are needed. I can open a PR once a new gimli version is released.

Wasmtime GitHub notifications bot (Apr 30 2020 at 04:23):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 30 2020 at 04:23):

peterhuene created PR Review Comment:

@philipc thanks for the heads up! When we update gimli, I think we can work around this by removing expression support from this representation since we don't need it to describe our frames currently.

Last updated: Nov 22 2024 at 16:03 UTC