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

Print sigfaults and exceptions to STDERR #4163

Merged
merged 1 commit into from Mar 20, 2017

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Mar 19, 2017

While working on raven.cr I've stumbled upon the problem of intercepting exceptions and sigfaults thrown by Crystal code and to my surprise I was unable to do so. After several hours of debugging I've found the problem in using LibC.printf.

This PR changes __crystal_raise, __crystal_sigfault_handler and CallStack.print_frame to use LibC.dprintf instead.

@Sija
Copy link
Contributor Author

Sija commented Mar 19, 2017

Also, while sitting on it, I was wondering does [%d] notation in CallStack.print_frame shouldn't be [0x%x] like in __crystal_sigfault_handler?

src/callstack.cr Outdated
if repeated_frame.count == 0
LibC.printf "[%ld] %s +%ld\n", repeated_frame.ip, sname, offset
STDERR.printf "[%d] %s +%d\n", repeated_frame.ip.address, sname, offset
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines use LibC.printf instead of printf in the stdlib to avoid the nonblocking IO code. I'd suggest using LibC.dprintf with fd 2, although you'll have to bind that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@RX14 do you mean fprintf?

Copy link
Contributor

Choose a reason for hiding this comment

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

fprintf expects a FILE*, while dprintf expects only a int fd, so I think the latter would be easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RX14 thanks for the tip, I've updated the PR accordingly. I was thinking of introducing an enum (STDIN, STDOUT, STDERR) for the fd argument to improve readability, WDYT?

Bind `LibC.dprintf` function
@bcardiff bcardiff added this to the Next milestone Mar 20, 2017
@bcardiff bcardiff merged commit 64e7b42 into crystal-lang:master Mar 20, 2017
@bcardiff
Copy link
Member

Thanks @Sija!

Regarding the pointers, probably the %x is better since they are expected for printing pointers. Feel free to handle that in another PR if you want, but it is not high prio.

@Sija
Copy link
Contributor Author

Sija commented Mar 21, 2017

I've pushed a followup #4170 PR with that change, cheers!

@Sija Sija mentioned this pull request Mar 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants