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

Various updates for the Julia integration #4042

Merged
merged 1 commit into from
May 28, 2020

Conversation

fingolfin
Copy link
Member

  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.

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 the safe_restore code completely, yet testinstall 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.

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.
@fingolfin fingolfin added topic: kernel topic: julia Julia GC integration and related matters backport-to-4.11 labels May 27, 2020
@fingolfin fingolfin requested a review from rbehrends May 27, 2020 22:31
@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.891% when pulling 75ec286 on fingolfin:mh/julia-tweaks into 8a0a129 on gap-system:master.

@rbehrends
Copy link
Contributor

The reason why we use safe_restore is to intercept segmentation faults. These are captured by Julia and passed on through the exception handling mechanism. Handling it via JL_TRY and JL_CATCH might be a possibility, but I'm not 100% sure that this will work in this context. I'll have to look closer at the code.

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:

+-------------------------------+
| guard | unused     | actual   |
| pages | stack area | stack    |
+-------------------------------+
                     ^
                     |
                  task sp

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.

@fingolfin
Copy link
Member Author

Yeah, I know why we need this in theory but in practice, I failed to trigger it.

@fingolfin fingolfin merged commit 872a8e2 into gap-system:master May 28, 2020
@fingolfin fingolfin deleted the mh/julia-tweaks branch May 28, 2020 13:34
@fingolfin
Copy link
Member Author

Backported to stable-4.11 in commit 27e4a5e

@PaulaHaehndel PaulaHaehndel self-assigned this Feb 16, 2021
@PaulaHaehndel PaulaHaehndel added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Feb 16, 2021
@PaulaHaehndel PaulaHaehndel removed their assignment Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.11-DONE release notes: added PRs introducing changes that have since been mentioned in the release notes topic: julia Julia GC integration and related matters topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants