Stream: git-wasmtime

Topic: wasmtime / PR #1572 Fix value label ranges resolution

Wasmtime GitHub notifications bot (Apr 21 2020 at 22:38):

yurydelendik opened PR #1572 from fix-value-label-resolve to master:

There was a bug how value labels were resolved, which caused some DWARF expressions not be transformed, e.g. those are in the registers.

• Patch built on top of #1542
• Documentation for ValueLocRange
• Proper logic in ValueLabelRangesBuilder::process_label
• Tests for ValueLabelRangesBuilder
• Refactor build_with_locals to return Iterator instead of Vec<_>

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

yurydelendik updated PR #1572 from fix-value-label-resolve to master:

There was a bug how value labels were resolved, which caused some DWARF expressions not be transformed, e.g. those are in the registers.

• Patch built on top of #1542
• Documentation for ValueLocRange
• Proper logic in ValueLabelRangesBuilder::process_label
• Tests for ValueLabelRangesBuilder
• Refactor build_with_locals to return Iterator instead of Vec<_>

Wasmtime GitHub notifications bot (Apr 28 2020 at 16:54):

yurydelendik updated PR #1572 from fix-value-label-resolve to master:

There was a bug how value labels were resolved, which caused some DWARF expressions not be transformed, e.g. those are in the registers.

• Patch built on top of #1542
• Documentation for ValueLocRange
• Proper logic in ValueLabelRangesBuilder::process_label
• Tests for ValueLabelRangesBuilder
• Refactor build_with_locals to return Iterator instead of Vec<_>

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:18):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:18):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:18):

ggreif created PR Review Comment:

                        .flat_map(move |(wasm_start, wasm_end)| {
                            addr_tr.translate_ranges(*wasm_start, *wasm_end)
                        })

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:18):

ggreif created PR Review Comment:

            // Starting from the end, intersect (range_start..range_end) with

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:18):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:22):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:22):

ggreif created PR Review Comment:

Courtesy of Haskell's concatMap :-)
Potentially a bit faster.

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:33):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:35):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:35):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:35):

bjorn3 created PR Review Comment:

Why not &'a [write::FileId]? Saves an indirection.

Wasmtime GitHub notifications bot (Apr 28 2020 at 20:59):

yurydelendik updated PR #1572 from fix-value-label-resolve to master:

There was a bug how value labels were resolved, which caused some DWARF expressions not be transformed, e.g. those are in the registers.

• Patch built on top of #1542
• Documentation for ValueLocRange
• Proper logic in ValueLabelRangesBuilder::process_label
• Tests for ValueLabelRangesBuilder
• Refactor build_with_locals to return Iterator instead of Vec<_>

Wasmtime GitHub notifications bot (Apr 28 2020 at 21:22):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Apr 28 2020 at 21:22):

ggreif created PR Review Comment:

        let value_ranges = self.frame_info
        .and_then(|fi| fi.value_ranges.get(&label))
        .unwrap_or({return});

This might be a little more clear. Matter of taste.

Wasmtime GitHub notifications bot (Apr 28 2020 at 21:24):

ggreif edited PR Review Comment.

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

yurydelendik updated PR #1572 from fix-value-label-resolve to master:

There was a bug how value labels were resolved, which caused some DWARF expressions not be transformed, e.g. those are in the registers.

• Patch built on top of #1542
• Documentation for ValueLocRange
• Proper logic in ValueLabelRangesBuilder::process_label
• Tests for ValueLabelRangesBuilder
• Refactor build_with_locals to return Iterator instead of Vec<_>

Wasmtime GitHub notifications bot (Apr 29 2020 at 14:43):

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 14:43):

ggreif created PR Review Comment:

It is probably evil. Sorry for this :-/
The only reference a search turned up was https://www.reddit.com/r/rust/comments/86x1fj/unwrap_orreturn/
and I am now convinced, that an eagerly evaluated argument return when passed to unwrap_or will exit the function in any case. I will learn a bit more about this antipattern, but don't apply it :skull:!

Wasmtime GitHub notifications bot (Apr 29 2020 at 14:56):

yurydelendik edited PR #1572 from fix-value-label-resolve to master:

There was a bug how value labels were resolved, which caused some DWARF expressions not be transformed, e.g. those are in the registers.

• Implements FIXME in expression.rs
• Move TargetIsa from CompliedExpression structure
• Fix expression format for GDB
• Add tests for parsing
• Proper logic in ValueLabelRangesBuilder::process_label
• Tests for ValueLabelRangesBuilder
• Refactor build_with_locals to return Iterator instead of Vec<_>
• Misc comments and magical numbers

Wasmtime GitHub notifications bot (Apr 29 2020 at 14:58):

ggreif deleted PR Review Comment.

Wasmtime GitHub notifications bot (Apr 29 2020 at 14:58):

ggreif deleted PR Review Comment.

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

yurydelendik edited PR #1572 from fix-value-label-resolve to master:

There was a bug how value labels were resolved, which caused some DWARF expressions not be transformed, e.g. those are in the registers.

• Implements FIXME in expression.rs
• Move TargetIsa from CompiledExpression structure
• Fix expression format for GDB
• Add tests for parsing
• Proper logic in ValueLabelRangesBuilder::process_label
• Tests for ValueLabelRangesBuilder
• Refactor build_with_locals to return Iterator instead of Vec<_>
• Misc comments and magical numbers

Wasmtime GitHub notifications bot (Apr 29 2020 at 15:13):

yurydelendik updated PR #1572 from fix-value-label-resolve to master:

There was a bug how value labels were resolved, which caused some DWARF expressions not be transformed, e.g. those are in the registers.

• Implements FIXME in expression.rs
• Move TargetIsa from CompiledExpression structure
• Fix expression format for GDB
• Add tests for parsing
• Proper logic in ValueLabelRangesBuilder::process_label
• Tests for ValueLabelRangesBuilder
• Refactor build_with_locals to return Iterator instead of Vec<_>
• Misc comments and magical numbers

Wasmtime GitHub notifications bot (Apr 29 2020 at 18:25):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 18:25):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 18:25):

fitzgen created PR Review Comment:

Tangent: Why aren't we doing #[derive(Clone)] for CompiledExpressionPart? I don't see anything here that is different from what the derive would do.

Wasmtime GitHub notifications bot (Apr 29 2020 at 18:25):

fitzgen created PR Review Comment:

nit: rm commented out code

Wasmtime GitHub notifications bot (Apr 29 2020 at 18:25):

fitzgen created PR Review Comment:

Why asserting the debug formatting and not the value itself?

Wasmtime GitHub notifications bot (Apr 29 2020 at 18:25):

fitzgen created PR Review Comment:

Why not move this if machine_reg < 32 logic into GimliWriter::write_op_reg? That could remove a precondition assertion from that method and some duplication here and below.

Wasmtime GitHub notifications bot (Apr 29 2020 at 18:25):

fitzgen created PR Review Comment:

    // Dereference is needed.

Wasmtime GitHub notifications bot (Apr 29 2020 at 23:17):

yurydelendik submitted PR Review.

Wasmtime GitHub notifications bot (Apr 29 2020 at 23:17):

yurydelendik created PR Review Comment:

Not sure. But it is gone: we have [derive(Clone)] now.

Wasmtime GitHub notifications bot (Apr 30 2020 at 01:52):

yurydelendik updated PR #1572 from fix-value-label-resolve to master:

There was a bug how value labels were resolved, which caused some DWARF expressions not be transformed, e.g. those are in the registers.

• Implements FIXME in expression.rs
• Move TargetIsa from CompiledExpression structure
• Fix expression format for GDB
• Add tests for parsing
• Proper logic in ValueLabelRangesBuilder::process_label
• Tests for ValueLabelRangesBuilder
• Refactor build_with_locals to return Iterator instead of Vec<_>
• Misc comments and magical numbers

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

ggreif submitted PR Review.

Wasmtime GitHub notifications bot (Apr 30 2020 at 13:07):

yurydelendik merged PR #1572.

Last updated: Nov 22 2024 at 17:03 UTC