Stream: git-wasmtime

Topic: wasmtime / PR #4469 cranelift: Add a new, optional flag f...

Wasmtime GitHub notifications bot (Jul 19 2022 at 21:46):

fitzgen opened PR #4469 from preserve-frame-pointers-flag to main:

This flag is optional, but when it is set then Cranelift backends must preserve frame pointers (or ISA equivalent).

Preserving frame pointers -- even inside leaf functions -- makes it easy to capture the stack of a running program, without requiring any side tables or metadata (like .eh_frame sections). Many sampling profilers and similar tools walk frame pointers to capture stacks. Enabling this option will play nice with those tools.

Our x64 backend already unconditionally preserves frame pointers, so it is trivially conforming with this new flag.

Our aarch64 backend preserves frame pointers or not based on whether the machinst framework tells it to or not by calling gen_prologue_frame_setup, and the machinst framework looks at this new flag when deciding whether to call the backend's implementation of gen_prologue_frame_setup, so it is conforming with this new flag.

However, our s390x backend has an empty implementation of gen_prologue_frame_setup, and so it is _not_ preserving frame pointers when this new flag is set. We will need a follow up PR that brings our s390x backend into conformance with this new flag.

cc @uweigand, your help with this last bit would be very appreciated, as I'm not familiar with s390x.

cc https://github.com/bytecodealliance/wasmtime/pull/4431

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jul 19 2022 at 21:46):

fitzgen requested cfallin for a review on PR #4469.

Wasmtime GitHub notifications bot (Jul 19 2022 at 22:11):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 19 2022 at 22:11):

cfallin created PR review comment:

Can we add the equivalent of this "preserves frame pointers" test for x64 too? The flag won't actually matter now but it'd be good to have the test coverage if/when we add any omit-frame-pointers mode in the future.

Wasmtime GitHub notifications bot (Jul 19 2022 at 22:11):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 19 2022 at 22:24):

fitzgen updated PR #4469 from preserve-frame-pointers-flag to main.

Wasmtime GitHub notifications bot (Jul 19 2022 at 22:25):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jul 19 2022 at 22:25):

fitzgen created PR review comment:

Done!

Wasmtime GitHub notifications bot (Jul 20 2022 at 15:02):

fitzgen merged PR #4469.

Wasmtime GitHub notifications bot (Jul 20 2022 at 16:22):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2022 at 16:22):

bjorn3 created PR review comment:

LLVM has a tri-state value of "always", "non-leaf" and "may-omit". It would be nice if Cranelift could have the same so I don't have to use the equivalent of "always" if "non-leaf" was chosen by the rustc target. For example Apple AArch64 targets require "non-leaf".

Wasmtime GitHub notifications bot (Jul 20 2022 at 16:57):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jul 20 2022 at 16:57):

fitzgen created PR review comment:

This is something we can do in the future as needed.

Wasmtime GitHub notifications bot (Jul 22 2022 at 13:56):

akirilov-arm created PR review comment:

Actually with the way the AArch64 backend works right now it is either "always" (i.e. preserve_frame_pointers being true) or "non-leaf" (i.e. false value). "may-omit" sounds as if it would be fine to treat it as "non-leaf".

Wasmtime GitHub notifications bot (Jul 22 2022 at 13:56):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 22 2022 at 14:05):

akirilov-arm edited PR review comment.

Wasmtime GitHub notifications bot (Jul 22 2022 at 14:19):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 22 2022 at 14:19):

bjorn3 created PR review comment:

The name preserve_frame_pointers makes it seem like false means that frame pointers may always be omitted.

Wasmtime GitHub notifications bot (Jul 26 2022 at 17:02):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Jul 26 2022 at 17:02):

akirilov-arm created PR review comment:

Yes, thinking a bit more about this, actually the AArch64 backend always "preserves" the frame pointer (i.e. it always has a valid value - either 0 or an address that points to a frame record on the stack), since the respective register is not allocatable; it is just that some functions (simple leaf functions that do not use the stack) opt not to update it, and keep its value as set by their callers.

Last updated: Oct 23 2024 at 20:03 UTC