Stream: git-wasmtime

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


view this post on Zulip 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:

Fixes #1181.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 03 2020 at 20:56):

peterhuene requested yurydelendik for a review on PR #1466.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip 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:

  1. 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
  2. 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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2020 at 20:30):

abrown submitted PR Review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2020 at 20:39):

peterhuene submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2020 at 20:40):

peterhuene edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 08 2020 at 20:41):

peterhuene submitted PR Review.

view this post on Zulip 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.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 14 2020 at 21:14):

peterhuene assigned PR #1466 to yurydelendik.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 14 2020 at 21:14):

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

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 16:06):

yurydelendik submitted PR Review.

view this post on Zulip 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).

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 19:14):

peterhuene submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 19:54):

abrown submitted PR Review.

view this post on Zulip 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!

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 20:17):

peterhuene submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 20:43):

peterhuene deleted PR Review Comment.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 21:03):

peterhuene submitted PR Review.

view this post on Zulip 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.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 21:59):

yurydelendik submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 22:53):

yurydelendik submitted PR Review.

view this post on Zulip 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).

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 22:54):

yurydelendik edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 23:01):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 15 2020 at 23:33):

peterhuene submitted PR Review.

view this post on Zulip 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.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 16 2020 at 04:34):

peterhuene submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 16 2020 at 09:12):

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

view this post on Zulip Wasmtime GitHub notifications bot (Apr 16 2020 at 14:02):

yurydelendik submitted PR Review.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip 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:

Fixes #1181.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 16 2020 at 20:34):

peterhuene merged PR #1466.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 28 2020 at 05:29):

philipc submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 28 2020 at 06:03):

philipc submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 30 2020 at 04:23):

peterhuene submitted PR Review.

view this post on Zulip 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: Dec 23 2024 at 12:05 UTC