Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Impl range write #362

Merged
merged 1 commit into from
Jan 19, 2019
Merged

Impl range write #362

merged 1 commit into from
Jan 19, 2019

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Jan 16, 2019 •
edited
Loading

Fixes #360

I have only implemented DWARF 4 .debug_ranges writing, not DWARF 5 .debug_rnglists yet.

Edit: implemented .debug_rnglists, but didn't test it. (I have no DWARF 5 supporting dwarfdump)

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jan 16, 2019

It is failing for https://github.com/gimli-rs/gimli/pull/362/files#diff-b0573ccce68b7d366aa6a11719034ce8R1815.

thread 'write::unit::tests::test_attribute_value' panicked at 'assertion failed: `(left == right)`
  left: `Attribute { name: DwAt(85), value: RangeListsRef(RangeId(1)) }`,
 right: `Attribute { name: DwAt(85), value: RangeListsRef(RangeId(0)) }`', src/write/unit.rs:1958:25

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jan 16, 2019

I found the problem.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jan 16, 2019 •
edited
Loading

I have tested .debug_ranges with cg_clif and it seems to work correctly. Could you please test .debug_rnglists for me, as I don't have DWARF 5 aware tools?

@coveralls
Copy link

coveralls commented Jan 16, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 87.506% when pulling 1553a19 on bjorn3:impl_range_write into f3116ec on gimli-rs:master.

@philipc
Copy link
Collaborator

philipc commented Jan 17, 2019

Could you please test .debug_rnglists for me, as I don't have DWARF 5 aware tools?

Changing version to 5 and running dwarfdump on target/out/mini_core.dummy_name.rcgu.o looks ok. Note that gimli's dwarfdump should be able to read everything we write.

@bjorn3 bjorn3 changed the title [WIP] Impl range write Impl range write Jan 17, 2019
@bjorn3
Copy link
Contributor Author

bjorn3 commented Jan 17, 2019

This is ready for review.

@bjorn3 bjorn3 mentioned this pull request Jan 17, 2019
Merged
@bjorn3
Copy link
Contributor Author

bjorn3 commented Jan 17, 2019

When enabling dwarf version 5 in cg_clif, I get Failed to dump attribute value: The end of an address range must not be before the beginning.:

dwarfdump target/out/mod_bench | grep -A 20 AT_range
                    DW_AT_ranges                0x0000000c
Failed to dump attribute value: The end of an address range must not be before the beginning.

LOCAL_SYMBOLS:
< 1><0x0000002a>    DW_TAG_subprogram
                      DW_AT_linkage_name          _ZN4core3num21_$LT$impl$u20$u32$GT$9max_value17hd2ebfb9c786040faE
                      DW_AT_name                  dummy_ZN4core3num21_$LT$impl$u20$u32$GT$9max_value17hd2ebfb9c786040faE
                      DW_AT_low_pc                0x000006c0
                      DW_AT_frame_base            len 0x0001: 56: DW_OP_reg6
                      DW_AT_decl_file             /home/bjorn/Documenten/rustc_codegen_cranelift/build_sysroot/sysroot_src/src/libcore/num/mod.rs
                      DW_AT_decl_line             0x00000888
                      DW_AT_decl_column           0x0000000c
                      DW_AT_high_pc               <offset-from-lowpc>39
< 2><0x0000004c>      DW_TAG_variable
                        DW_AT_name                  <no name>
                        DW_AT_type                  <0x00000059>
                        DW_AT_location              len 0x0002: 916c: DW_OP_fbreg -20
< 1><0x00000059>    DW_TAG_base_type
                      DW_AT_name                  u32
                      DW_AT_encoding              DW_ATE_unsigned
                      DW_AT_byte_size             0x00000004
--
                    DW_AT_ranges                0x00000216
                ranges: 1785 at .debug_rnglists offset 534 (0x00000216)
                        [ 0] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [ 1] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [ 2] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [ 3] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [ 4] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [ 5] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [ 6] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [ 7] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [ 8] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [ 9] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [10] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [11] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [12] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [13] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [14] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [15] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [16] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [17] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>
                        [18] <start-end low-off: 0x00000000 addr 0x00000000 high-off: 0x00000000 addr 0x00000000>

philipc
philipc requested changes Jan 18, 2019
View reviewed changes
Copy link
Collaborator

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! Most of the comments are minor.

src/write/range.rs Outdated Show resolved Hide resolved
src/write/range.rs
/// A table of ranges that will be stored in a `.debug_ranges` or `.debug_rnglists` section.
#[derive(Debug, Default)]
pub struct RangesTable {
ranges: IndexSet<RangeList>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ranges: IndexSet<RangeList>,
// TODO: determine if duplicate range lists occur in practice. This may be better as a plain `Vec`.
ranges: IndexSet<RangeList>,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean to improve performance?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Probably won't make much difference to performance in the whole either. Mostly I just want to document that use of IndexSet probably isn't strictly required.

src/write/range.rs Outdated Show resolved Hide resolved
src/write/range.rs Outdated Show resolved Hide resolved
src/write/range.rs Outdated Show resolved Hide resolved
src/write/unit.rs Outdated Show resolved Hide resolved
src/write/range.rs Outdated Show resolved Hide resolved
src/write/range.rs Outdated Show resolved Hide resolved
src/write/range.rs Outdated Show resolved Hide resolved
src/write/unit.rs Outdated Show resolved Hide resolved
@philipc
Copy link
Collaborator

philipc commented Jan 18, 2019

This won't support conversion of relocatable range lists. For that we'll need to switch to converting the raw ranges, but I'm happy to leave that until another PR.

Normally I would also want more tests in write::range, but those tests will need significant changes for the raw range support, so it's okay to skip that for now.

@philipc
Copy link
Collaborator

philipc commented Jan 18, 2019 •
edited
Loading

This is looking good. Can you please:

  • fix the failing test (by allowing .debug_ranges for DWARF version 2)
  • run rustfmt
  • squash into logical commits (one commit is probably fine)

philipc
philipc reviewed Jan 19, 2019
View reviewed changes
src/write/range.rs Outdated Show resolved Hide resolved
@bjorn3
Copy link
Contributor Author

bjorn3 commented Jan 19, 2019

Done

@philipc philipc merged commit 9930c61 into gimli-rs:master Jan 19, 2019
@philipc
Copy link
Collaborator

philipc commented Jan 19, 2019

Thanks!

@bjorn3 bjorn3 deleted the impl_range_write branch January 19, 2019 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipc philipc philipc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bjorn3 @coveralls @philipc