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

julia: avoid access into Julia structs in Julia >= 1.6 #4086

Merged
merged 2 commits into from
Oct 7, 2020

Conversation

fingolfin
Copy link
Member

We currently access members of various structs provided by Julia (of type jl_task_t resp. jl_tls_t). This introduces an unwanted binary dependency on the precise Julia version GAP was compiled against; i.e. because of these, a binary compiled against Julia 1.3 cannot be used with Julia 1.5

To avoid this, the Julia folks merged some new APIs:

In order to simultaneously support older and newer Julia versions, we use two tricks:

  • we don't link during compile time against the new APIs; instead we use dlsym to query at runtime if they are available, and only then call them (this introduces as small overhead at the call sites, but this is negligible)
  • for task->copy_stack this is enough as its offset has been unchanged between Julia 1.3 and Julia 1.6
  • for tls->safe_restore, the offset did change once between Julia 1.4 and 1.5. Luckily, the change is the same on all 64 bit platforms: the offset shifted by 8. To adjust for that, we compare the Julia version used to compile GAP against that which runs GAP to compute a suitable offset for adjusting access to tls->safe_restore.

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: julia Julia GC integration and related matters backport-to-4.11 labels Jul 30, 2020
@fingolfin fingolfin requested a review from rbehrends July 30, 2020 15:55
@coveralls
Copy link

coveralls commented Jul 30, 2020

Coverage Status

Coverage decreased (-0.001%) to 93.728% when pulling 5fa6e0b on fingolfin:mh/julia-no-struct-access into 9b20de7 on gap-system:master.

@fingolfin fingolfin force-pushed the mh/julia-no-struct-access branch 3 times, most recently from b6fca4f to 51bb7c3 Compare July 31, 2020 23:32
Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, given the commit messages.

@fingolfin
Copy link
Member Author

This needs to be updated to reflect the changes in JuliaLang/julia#36823

It was previously turned off as an unintended side effect of adding
`--disable-Werror` to the configure flags (as the `libuv.h` from Julia
introduced some warnings)
We currently access members of various structs provided by Julia (of
type jl_task_t resp. jl_tls_t). This introduces an unwanted binary
dependency on the precise Julia version GAP was compiled against; i.e.
because of these, a binary compiled against Julia 1.3 cannot be used
with Julia 1.5

To avoid this, the Julia folks merged some new APIs:
- `jl_active_task_stack` allows us to avoid access to `task->copy_stack`
- `jl_get_safe_restore` and `jl_set_safe_restore` allow us to avoid
  direct access to `tls->safe_restore`

In order to simultaneously support older and newer Julia versions, we
use two tricks:
- we don't link during compile time against the new APIs; instead we use
  `dlsym` to query at runtime if they are available, and only then call
  them (this introduces as small overhead at the call sites, but this is
  negligible)
- for `task->copy_stack` this is enough as its offset has been unchanged
  between Julia 1.3 and Julia 1.6
- for `tls->safe_restore`, the offset *did* change once between Julia 1.4
  and 1.5. Luckily, the change is the same on all 64 bit platforms: the
  offset shifted by 8. To adjust for that, we compare the Julia version
  used to compile GAP against that which runs GAP to compute a suitable
  offset for adjusting access to `tls->safe_restore`.
@fingolfin fingolfin force-pushed the mh/julia-no-struct-access branch from 51bb7c3 to 5fa6e0b Compare October 7, 2020 14:30
@fingolfin
Copy link
Member Author

Rebased and adapted to match the final version of jl_active_task_stack which was merged into Julia in PR JuliaLang/julia#36823. Assuming this passes, we can finally merge this PR and then backport it to stable-4.11

@ThomasBreuer ping just FYI

@fingolfin fingolfin merged commit 022d69b into gap-system:master Oct 7, 2020
@fingolfin fingolfin deleted the mh/julia-no-struct-access branch October 7, 2020 20:10
@fingolfin
Copy link
Member Author

Backported to stabled-4.11 in ef2ad98

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: not needed PRs introducing changes that are wholly irrelevant to 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