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

Gracefully handle owee failures in Elf.addr_table #195

Merged
merged 2 commits into from
May 12, 2022

Conversation

cgaebel
Copy link
Contributor

@cgaebel cgaebel commented Apr 25, 2022

Owee shouldn't have the ability to crash magic-trace. It's ~always preferably to not display debug info instead of crashing on users.

@cgaebel cgaebel requested review from Xyene and removed request for Xyene April 25, 2022 15:44
@cgaebel cgaebel marked this pull request as ready for review April 25, 2022 16:29
Copy link
Member

@Xyene Xyene left a comment

Choose a reason for hiding this comment

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

LGTM, but we should probably warn when this happens, to disambiguate between "don't have debug info installed at all" and "have debug info but magic-trace can't interpret it"?

magic-trace only really needs debug symbols to choose a snapshot
symbol and to provide file and line numbers in the trace. Make it
so that if debug symbols are missing, we disable dependent functionality
and issue a warning.

Fixes janestreet#191
@cgaebel
Copy link
Contributor Author

cgaebel commented May 5, 2022

I reworked this, please take another look.

src/trace.ml Outdated Show resolved Hide resolved
Copy link
Member

@Xyene Xyene left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM, thanks.

Co-authored-by: Tudor Brindus <vulcainus@gmail.com>
@cgaebel cgaebel merged commit 7ecf839 into janestreet:master May 12, 2022
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.

None yet

2 participants