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

Exception::CallStack: avoid allocations in LibC.dl_iterate_phdr #12625

Merged
merged 1 commit into from
Nov 12, 2022
Merged

Conversation

dmgk
Copy link
Contributor

@dmgk dmgk commented Oct 18, 2022

Calling self.read_dwarf_sections directly from the C callback may lead to reallocations and deadlocks due to the internal lock held by dl_iterate_phdr (#10084). Work around this by storing object base address and passing it to self.read_dwarf_sections later when dl_iterate_phdr returns.

This also fixes #12329 because there's no need to disable GC around dl_iterate_phdr anymore.

Calling self.read_dwarf_sections directly from the C callback may lead
to reallocations and deadlocks due to the internal lock held by
dl_iterate_phdr (#10084). Work around this by storing object base
address and passing it to self.read_dwarf_sections later when
dl_iterate_phdr returns.

This also fixes #12329 because there's no need to disable GC around
dl_iterate_phdr anymore.
@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:runtime platform:freebsd labels Oct 18, 2022
@@ -5,16 +5,19 @@ require "crystal/elf"

struct Exception::CallStack
protected def self.load_debug_info_impl
base_address : LibC::Elf_Addr = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid using a TypeDeclaration here unless necessary:

Suggested change
base_address : LibC::Elf_Addr = 0
base_address = LibC::Elf_Addr.zero

Copy link
Member

Choose a reason for hiding this comment

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

Later we're passing a pointer to this variable. So IMO it makes sense to pin its type in order to prevent an accidental union type tainting the pointer.

@beta-ziliani
Copy link
Member

Is it OK to only call read_dwarf_sections with the last address provided by dl_iterate_phdr? IIUC, that's what this change is introducing. Before it was calling read_dwarf_sections for each object loaded.

@dmgk
Copy link
Contributor Author

dmgk commented Nov 10, 2022

@beta-ziliani No, the original code was also stopping after reading the first object. Quoting dl_iterate_phdr(3):

The dl_iterate_phdr() function walks through the list of an
application's shared objects and calls the function callback once
for each object, until either all shared objects have been
processed or callback returns a nonzero value.

@beta-ziliani
Copy link
Member

Totally missed that, thanks!

@beta-ziliani beta-ziliani added this to the 1.7.0 milestone Nov 10, 2022
@straight-shoota straight-shoota changed the title Exception::CallStack: avoid allocations in LibC.dl_iterate_phdr callback Exception::CallStack: avoid allocations in LibC.dl_iterate_phdr Nov 12, 2022
@straight-shoota straight-shoota merged commit 6edc9e3 into crystal-lang:master Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:freebsd topic:stdlib:runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception handling miscompilation on FreeBSD
4 participants