Stream: git-wasmtime

Topic: wasmtime / PR #6931 Two fixes for the `-g` (debug info) o...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 12:36):

SingleAccretion edited PR #6931:

Two changes:
1) Fix a small bug where a constant DWARF "block" wasn't handled in transformation.
2) Fix up instruction offsets after code emission to account for branch removal.

See also the commit messages for a bit more detail.

With these two changes, I am able to step through the code generated by the compiler we are working on.

Fixes #3999.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 12:55):

SingleAccretion updated PR #6931.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 12:57):

SingleAccretion has marked PR #6931 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 12:57):

SingleAccretion requested fitzgen for a review on PR #6931.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 12:57):

SingleAccretion requested wasmtime-compiler-reviewers for a review on PR #6931.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 12:57):

SingleAccretion requested wasmtime-default-reviewers for a review on PR #6931.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 12:57):

SingleAccretion requested wasmtime-core-reviewers for a review on PR #6931.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 16:20):

fitzgen submitted PR review:

Looks good to me!

Would it be possible to construct a somewhat minimal test case where the branch lowering messes up the offsets and we need to monotize them and add it to our debugging tests? Eg https://github.com/bytecodealliance/wasmtime/blob/main/tests/all/debug/lldb.rs#L62

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 16:28):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 16:30):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 16:33):

cfallin created PR review comment:

Hmm -- it's theoretically possible, if it were a block that had just a jump instruction, no fallthrough (previous block ended in an uncond branch), and we rewrote branches to this last block to go directly to the jump dest instead.

Perhaps you could pull out the result of buffer.finish(...) (which is where the last optimization call occurs) to before this line?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 16:47):

cfallin created PR review comment:

This does seem to leave an interesting corner case: if the very first instruction is a jump and is chomped (say because it's a jump to the fallthrough location, i.e. the next block), we artificially skip.

However I think it's OK for the following very subtle reason: we can only chomp back to an empty buffer (and hence have an offset of 0 on the non-first instruction) if we have one jump instruction (and nothing else) in the first block; MachBuffer will never chomp e.g. a cond+uncond pair to nothing. In the single-jump-inst case, that inst also has an offset of 0 so it's still monotonic.

Could we leave a comment to this effect?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 16:57):

SingleAccretion submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 16:59):

SingleAccretion created PR review comment:

That is a great point you bring up.

if we have one jump instruction (and nothing else) in the first block; MachBuffer will never chomp e.g. a cond+uncond pair to nothing. In the single-jump-inst case, that inst also has an offset of 0 so it's still monotonic.

Am I right to understand that this is because of the VCode construction? I considered:

Block0:
  jmp Block1
  jmp Block1
Block1:

; Which _can_ be simplified down to nothing.

I was writing the comment as follows, does that match your expectation?

// Not all instructions get their offsets recorded. As for the case where
// the offset is legitimately zero (first instruction), we are guaranteed
// that no more than one prior jump (which would also have been at a zero
// offset) could have been removed because the first block will not have
// multiple jumps.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 17:00):

SingleAccretion edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 17:04):

SingleAccretion edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 17:31):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 17:35):

cfallin created PR review comment:

I would replace the last bit with "... because either the first block will not have multiple branches, or else it would not have been completely elided."

This is related to your example: no block will have two unconditional branches; we emit only one "terminator" VCode instruction per block, and those are either one uncond, or a two-target form that is cond+uncond (or switch or ret, both of which are indirect).

(Additional subtlety just to allay any confusion: some pseudoinsts do emit branch insts internally, but those are completely invisible to the MachBuffer (i.e., not reported).)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 18:22):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 18:32):

SingleAccretion created PR review comment:

I've thought about this some more, and became less convinced it's a correct assumption. Consider:

Block0:
  jmp Block2
Block1:
  jmp Block2
Block2:

(This can be fixed in a more obvious matter by using not-zero for the distinguished "invalid offset" value)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 18:43):

SingleAccretion edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 19:21):

SingleAccretion created PR review comment:

These Unix exclusions are copy&pasted with the assumption they're CI-induced. I verified the test passes on Windows.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 19:21):

fitzgen created PR review comment:

Yeah it is a bit funky, but these tests are only force-enabled in some CI configurations.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 19:21):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 19:23):

SingleAccretion updated PR #6931.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 19:35):

SingleAccretion submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 19:44):

SingleAccretion submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 19:48):

cfallin created PR review comment:

Ah! Yes, indeed, that's the far more sensible solution. (One might be tempted to make it a Vec<Option<CodeOffset>> but for memory-efficiency let's define a sentinel value e.g. const NO_INST_OFFSET: CodeOffset = u32::MAX maybe?)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2023 at 20:14):

SingleAccretion updated PR #6931.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 01 2023 at 16:39):

cfallin submitted PR review:

Latest changes LGTM to me -- thanks very much for this!

view this post on Zulip Wasmtime GitHub notifications bot (Sep 01 2023 at 17:42):

cfallin merged PR #6931.

Last updated: Nov 22 2024 at 17:03 UTC