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

Fix stack scanning for the Julia GC when GAP is used as a library. #3432

Merged
merged 2 commits into from
May 4, 2019

Conversation

rbehrends
Copy link
Contributor

@rbehrends rbehrends commented May 2, 2019

Fix stack scanning for the Julia GC when GAP is used as a library.

When GAP is being initialized from GAP.jl, we cannot use GapStackBottom to figure out the start of the stack, but have to query Julia for it. Conversely, when GAP is the main program, we cannot rely on Julia knowing the start and end of the stack of the main thread.

This pull request introduces a flag, called UsingLibGap, which allows us to distinguish between the two cases and then calculates the stack extent accordingly.

Checklist for pull request reviewers

  • proper formatting

If your code contains kernel C code, run clang-format on it; the
simplest way is to use git clang-format, e.g. like this (don't
forget to commit the resulting changes):

git clang-format $(git merge-base master)
  • usage of relevant labels

    1. either release notes: not needed or release notes: to be added
    2. at least one of the labels bug or enhancement or new feature
    3. for changes meant to be backported to stable-4.X add the backport-to-4.X label
    4. consider adding any of the labels build system, documentation, kernel, library, tests
  • runnable tests

  • adequate pull request title

  • well formulated text for release notes

  • relevant documentation updates

  • sensible comments in the code

@coveralls
Copy link

coveralls commented May 2, 2019

Coverage Status

Coverage decreased (-0.0002%) to 84.991% when pulling 52bff6e on rbehrends:julia-gc-fix-stack into dbfbba7 on gap-system:master.

Copy link
Member

@sebasguts sebasguts left a comment

Choose a reason for hiding this comment

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

Overall, this solution looks good to me, thank you :).

I am actually kind of puzzled that we didn't had such a variable before, but I cannot find one, so I guess it is not there.

@fingolfin
Copy link
Member

This seems to be very tightly related to issue #3089, except that you only solve a very special case of that here.

@ThomasBreuer
Copy link
Contributor

The problem with

using GAP, Nemo
GAP.Globals.<TAB>

disappears after the changes from this pull request.

Also in other examples, the effect that Julia freezes at some point disappears.

src/gap.h Outdated Show resolved Hide resolved
src/julia_gc.c Outdated Show resolved Hide resolved
src/julia_gc.c Show resolved Hide resolved
src/gap.h Outdated Show resolved Hide resolved
src/gap.h Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 3, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@dbfbba7). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #3432   +/-   ##
=========================================
  Coverage          ?   85.17%           
=========================================
  Files             ?      699           
  Lines             ?   346082           
  Branches          ?        0           
=========================================
  Hits              ?   294785           
  Misses            ?    51297           
  Partials          ?        0
Impacted Files Coverage Δ
src/gap.c 81.55% <ø> (ø)
src/libgap-api.c 62.8% <100%> (ø)
src/julia_gc.c 77.4% <100%> (ø)

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes topic: julia Julia GC integration and related matters labels May 3, 2019
@ChrisJefferson ChrisJefferson merged commit 2ebcef9 into gap-system:master May 4, 2019
@fingolfin fingolfin deleted the julia-gc-fix-stack branch June 4, 2019 11:53
@fingolfin fingolfin mentioned this pull request Jun 4, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.2 milestone Jun 12, 2019
@fingolfin fingolfin modified the milestone: GAP 4.10.2 Jun 13, 2019
@olexandr-konovalov olexandr-konovalov added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Jun 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.10-DONE kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes topic: julia Julia GC integration and related matters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants