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

RAI: do not prepend thread ID to backtraces from signal handler context #148

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

kpamnany
Copy link
Collaborator

@kpamnany kpamnany commented Apr 18, 2024

PR Description

Ctrl+T sends a SIGINFO (on Mac, SIGUSR1 on Linux) to Julia which triggers a backtrace. RAI Julia prepends [signal (X)] thread (Y) to backtraces. But in signal handler context, there is no "current task" from which to get the thread ID.

This PR fixes the problem by checking to see if we're in signal handler context before calling jl_threadid().

Checklist

Requirements for merging:

@github-actions github-actions bot added port-to-v1.10 This change should apply to Julia v1.10 builds port-to-master This change should apply to all future Julia builds labels Apr 18, 2024
Copy link
Member

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

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

🤔 wasn't the whole point of these thread-ids to make it easier to read segfaults when there are multiple threads? Those are also signal handlers, right?

(Did we never end up up-streaming that change?)

@kpamnany
Copy link
Collaborator Author

kpamnany commented Apr 18, 2024

🤔 wasn't the whole point of these thread-ids to make it easier to read segfaults when there are multiple threads? Those are also signal handlers, right?

Yes. But SIGSEGV and SIGBUS have a different signal handler function and that checks jl_current_task and takes an alternate path if it is NULL.

Ctrl+T is handled by the signal listener function, which directly dumps a backtrace here.

I tried checking jl_current_task the same way segv_handler does but it was 0xfffff...fe rather than NULL. That's because jl_current_task uses container_of on jl_get_pgcstack (which gets thread-local data) and container_of uses offsetof and NULL minus some offset will give you 0xfffff...<something>. I do not know why checking jl_current_task directly works for segv_handler.

In the process of writing this up, I found jl_get_current_task() which is better to use than jl_get_pgcstack() so I'll push that fix shortly.

(Did we never end up up-streaming that change?)

We did not. Jameson wanted the task pointer to be prepended instead of the thread ID and we never got around to arguing it. Edited to add: upstream issue is here.

Copy link
Member

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

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

Cool, thank you for the explanation! 👍

I don't see anything wrong with printing the task_id instead, if that's what jameson prefers? We just wanted to be able to separate the logs, i think?

@kpamnany kpamnany merged commit 01b4a24 into v1.10.2+RAI Apr 19, 2024
5 checks passed
@kpamnany kpamnany deleted the kp-fix-ctrl-t branch April 19, 2024 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
port-to-master This change should apply to all future Julia builds port-to-v1.10 This change should apply to Julia v1.10 builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants