-
Notifications
You must be signed in to change notification settings - Fork 162
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
Various updates for the Julia integration #4042
Conversation
1. test against Julia 1.4.2 instead of 1.2.0 2. fix compiler constness warnings in weakptr.c 3. use `jl_threadid()` and `jl_get_current_task()` helpers to avoid access to `JuliaTLS` members Point 3 is important because the layout of the `jl_ptls_t` struct keeps changing between Julia versions, meaning that one has to recompile GAP when updating Julia. With this patch, we avoid accessing the `tid` and `current_task` members. However, we still access `root_task` and `safe_restore`. More work will be needed to deal with those.
The reason why we use Why do we need this? Because some thread/task stacks have guard pages at the bottom. These are part of the stack area and have an unknown size. Accessing them will trigger a segmentation fault. Consider the following layout:
Here's where it gets complicated. We sometimes know the actual bottom of the stack (i.e. the bottommost stack frame) and sometimes we don't. If we do, we don't need to intercept SEGV signals, as they won't occur; but if we overrun the bottom stack frame and hit guard pages, then we need to. I.e. the question is whether we actually know the task sp in the above layout and can terminate the scan before we hit the guard pages. I'll have to look at the exact situation where that happens, though, as I don't recall the precise circumstances off-hand. |
Yeah, I know why we need this in theory but in practice, I failed to trigger it. |
Backported to stable-4.11 in commit 27e4a5e |
jl_threadid()
andjl_get_current_task()
helpers to avoid access toJuliaTLS
membersPoint 3 is important because the layout of the
jl_ptls_t
struct keepschanging between Julia versions, meaning that one has to recompile GAP when
updating Julia. With this patch, we avoid accessing the
tid
andcurrent_task
members.
However, we still access
root_task
andsafe_restore
. More work will be neededto deal with those.
For
root_task
(unfortunately, Julia doesn't provide an accessor function for this -- yet) luckily this is of no consequence in many cases: namely, if GAP is used as a library (i.e. loaded from Julia), that check is never executed; and if GAP loads Julia, then it ought to load the correct version of Julia anyway (i.e., for people who have multiple Julia versions installed in parallel, the right one is picked). It still might break if the user upgrades the Julia installation in place.For
safe_restore
, I wonder if we can perhaps get away with using JL_TRY / JL_CATCH. However, I just removed thesafe_restore
code completely, yettestinstall
still passed, @rbehrends do you remember by chance which crash this fixed? The code was originally inserted in PR #2969 but doesn't mention explicitly what the issue was / how to reproduce it.