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

Add UnwindTable::into_current_row #557

Merged
merged 2 commits into from
Jun 4, 2021
Merged

Add UnwindTable::into_current_row #557

merged 2 commits into from
Jun 4, 2021

Conversation

koute
Copy link
Contributor

@koute koute commented Jun 4, 2021

This will make it possible to drop the UnwindTable and return a reference to UnwindTableRow whose lifetime will be bound only to the lifetime of the UninitializedUnwindContext. This is still not perfect though since the UnwindTable constructor originally accepts the unwind context through a &mut, so you still won't be able to e.g. search through multiple sections and return a reference to UnwindTableRow for only one of them due to the limitations of Rust's current borrow checker without laundering the lifetime through unsafe, but it's better than what we have now.

(Some background on this change: I've been refactoring this horrible method from my profiler, and what I'd like to do is basically to look through multiple sections, and return the UnwindTableRow when I get a hit. I could just clone it, but it's over 6 kilobytes so if the compiler won't be able to elide that copy the performance will tank.)

An alternative to this would be to not store the UnwindContext inside of UnwindTable and just pass the unwind context manually each time we iterate over the rows instead of setting it once in the constructor. That would obviously be a more intrusive change than this one though, so I'm not sure if we actually want to go that way? (Especially since the new Polonius-based borrow checker fixes the issue I've described in the first paragraph.)

@philipc
Copy link
Collaborator

philipc commented Jun 4, 2021

An alternative to this would be to not store the UnwindContext inside of UnwindTable and just pass the unwind context manually each time we iterate over the rows instead of setting it once in the constructor.

Does this even avoid the lifetime problem? I experimented to get code like this in unwind_info_for_address:

        while let Some(row) = table.next_row(ctx)? {
            if row.contains(address) {
                return Ok(row);
            }
        }

but that gave a lifetime error.

I think the ideal API would be for next_row to return a reference with the context lifetime, but I'm not sure that's possible. Otherwise I think what you've done in this PR is fine. It would be good to change unwind_info_for_address to use this too though, since currently it is cloning the row.

@koute
Copy link
Contributor Author

koute commented Jun 4, 2021

Does this even avoid the lifetime problem? I experimented to get code like this in unwind_info_for_address:

Currently it doesn't, mostly due to the borrow checker limitations that won't be fixed until Polonius lands. (If you turn on -Zpolonius it will compile.) But by splitting it like that you could call ctx.row() directly after advancing the row since the ctx wouldn't be stored inside of the UnwindTable anymore.

I think the ideal API would be for next_row to return a reference with the context lifetime, but I'm not sure that's possible.

Yeah, I don't think that's currently possible.

Otherwise I think what you've done in this PR is fine. It would be good to change unwind_info_for_address to use this too though, since currently it is cloning the row.

Sure, I'll change it.

@koute
Copy link
Contributor Author

koute commented Jun 4, 2021

Done.

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!

@philipc philipc merged commit 4ea297a into gimli-rs:master Jun 4, 2021
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