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

kernel: when using julia_gc, call jl_atexit_hook in SyExit #2931

Merged
merged 2 commits into from
Oct 19, 2018

Conversation

fingolfin
Copy link
Member

... as recommended by https://docs.julialang.org/en/v1/manual/embedding/index.html

This makes it possible to collect coverage data in Julia code (as that is written to disk via that atexit hook).

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes backport-to-4.10 labels Oct 18, 2018
@ChrisJefferson
Copy link
Contributor

The changes to Panic/SyExit are sensible and good.

One random question -- why doesn't julia_gc just pass jl_atexit_hook to atexit?

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.

This looks sensible

src/saveload.c Outdated
@@ -374,8 +374,7 @@ static void LoadBagData ( void )

if (TNAM_TNUM(type) == NULL)
{
Pr("Bad type %d, size %d\n",type,size);
exit(1);
Panic("Bad type %d, size %d\n",type,size);
Copy link
Member

Choose a reason for hiding this comment

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

The compiler seems to complain about the %d

@fingolfin
Copy link
Member Author

@ChrisJefferson fair question, I also wondered. One thing is that jl_atexit_hook expects the exit code as an argument, which is (AFAIK) unknown in regular atexit handlers. I am not aware of other reasons, but that doesn't mean there aren't any. I'll try to find out.

@ChrisJefferson
Copy link
Contributor

Wanting the exit code is a good enough reason, in that case I'm happy to merge it (it's not going to effect GAP without Julia anyway of course)

@fingolfin fingolfin merged commit 9d49231 into gap-system:master Oct 19, 2018
@fingolfin fingolfin deleted the mh/jl_atexit_hook branch October 19, 2018 14:08
@fingolfin
Copy link
Member Author

Backported via 19aafe0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.10-DONE release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants