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

gc: add missing root for binding->ty field #46806

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

staticfloat
Copy link
Member

Previously, we might observe this code segfault if the memory at binding->ty eventually was reused due to this missing GC root.

julia> x::Union{Int,Nothing} = 2
2

julia> GC.gc()

julia> Core.get_binding_type(Main, :x)
Union{Nothing, Int64}

NOTE: I opened this PR on Jameson's behalf; the branch name in #46804 broke cloning from windows, so I moved the commits to a new branch and deleted his branch.

@gbaraldi
Copy link
Member

gbaraldi commented Sep 16, 2022

Could we add a test for this?

Edit: I'm not sure this works

julia> x::Union{Int,Nothing} = 2
2

julia> GC.gc()
GC error (probable corruption) :
Allocations: 322489 (Pool: 322233; Big: 256); GC: 0
0
0x123bf4000: Root object: 0x10a984010 :: 0x119cf4420 (bits: 1)
        of type Task
0x123bf4018: Root object: 0x10a9844c0 :: 0x119cf4420 (bits: 1)
        of type Task
0x123bf4030: Root object: 0x10aa74010 :: 0x119cf4420 (bits: 1)
        of type Task
0x123bf4048: Root object: 0x10aa7c010 :: 0x119cf4420 (bits: 1)
        of type Task
0x123bf4060: Root object: 0x10a9841a0 :: 0x119cf4420 (bits: 1)
        of type Task
0x123bf4078: Root object: 0x10aa78010 :: 0x119cf4420 (bits: 1)
        of type Task
0x123bf4090:  r-- Module (bindings) 0x119f7afe0 (bits 3) -- [0x14f9101d8, 0x14f910400)

[11046] signal (6): Abort trap: 6
in expression starting at REPL[2]:1
__pthread_kill at /usr/lib/system/libsystem_kernel.dylib (unknown line)
Allocations: 322489 (Pool: 322233; Big: 256); GC: 0
fish: Job 1, './julia' terminated by signal SIGABRT (Abort)

@giordano giordano added GC Garbage collector bugfix This change fixes an existing bug backport 1.8 Change should be backported to release-1.8 labels Sep 16, 2022
src/gc.c Outdated Show resolved Hide resolved
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Sep 29, 2022
@gbaraldi
Copy link
Member

This still fails for me.

@vchuravy vchuravy added this to the 1.9 milestone Oct 6, 2022
@vchuravy vchuravy merged commit 45b96c4 into master Oct 6, 2022
@vchuravy vchuravy deleted the sf/jn/binding_ty_root_backup branch October 6, 2022 19:10
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Oct 6, 2022
@KristofferC
Copy link
Member

Would be good if someone addressed #46806 (comment) to not leave it hanging.

@gbaraldi
Copy link
Member

gbaraldi commented Oct 6, 2022

The test case didn't fail for me anymore. But maybe adding a test would be nice

KristofferC pushed a commit that referenced this pull request Oct 27, 2022
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
(cherry picked from commit 45b96c4)
KristofferC pushed a commit that referenced this pull request Oct 28, 2022
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
(cherry picked from commit 45b96c4)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug GC Garbage collector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants