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.

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

fix(debuginfo): Three-way split line records for inlinee-ranges #239

Merged
merged 2 commits into from
May 27, 2020

Conversation

calixteman
Copy link
Contributor

@calixteman calixteman commented May 26, 2020

When an inline range is strictly included in a parent line record, it must be split into three parts and the end of the original line record must be retained for the last part. This fixes two distinct issues in the previous implementation:

  1. The last record was always aligned with the end of the inlinee-range, which either made it too long or left a gap.
  2. The third split was also patched with call site information, instead of retaining the original line info.

Before patching:

          +-------------------+
 Parent:  |         P         |
          +---+-----------+---+
Inlinee:      |     I     |
              +-----------+

After patching:

          +---+-----------+---+
 Parent:  | P |     C     | P |
          +-------------------+
Inlinee:      |     I     |
              +-----------+

(P parent line record, C Call site record, I inlinee line record)

Follow-up on #237

@calixteman calixteman requested a review from a team May 26, 2020 11:24
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Thanks, good catch! I checked for this case yesterday, but missed that we create a hole in line records. The bug comes from these lines:

let size = match lines.get(index) {
Some(next) => range_end.min(next.address) - range_begin,
None => range_end - range_begin,
};

We cap the size of the second half of the line record to the end of the range, but it should use max(size, range_end - start) instead. Do you concur? Changing this one line would probably be a more minimal diff.

debuginfo/src/dwarf.rs Show resolved Hide resolved
@jan-auer
Copy link
Member

After discussing this for a while, we figured that your initial patch is indeed the correct solution for this, with one minor change to calculate the length when splitting at the beginning:

let size = record_end.min(range_end) - range_begin;

In the future, a better approach would be to change this even more:

  • Operate on ranges with a start and end, rather than start and size. We could use Rust's builtin ranges for this.
  • Factor out the range intersection logic into a reusable component that can be unit-tested

In another step, we can consider if we even need to patch the parent line records nowadays.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Re-approving after walking through this together with @calixteman

@jan-auer jan-auer changed the title fix(debuginfo): parent range must be splited into 3 parts when an inl… fix(debuginfo): Split parent line records for sub-ranges May 27, 2020
@jan-auer jan-auer changed the title fix(debuginfo): Split parent line records for sub-ranges fix(debuginfo): Three-way split line records for inlinee-ranges May 27, 2020
@jan-auer jan-auer merged commit 24bfa45 into getsentry:master May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants