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

Root dt during construction in interpreter #25122

Merged
merged 1 commit into from
Dec 16, 2017
Merged

Root dt during construction in interpreter #25122

merged 1 commit into from
Dec 16, 2017

Conversation

Keno
Copy link
Member

@Keno Keno commented Dec 15, 2017

ClangSA was unhappy about this:

/home/keno/julia/src/interpreter.c:239:10: note: Started tracking value here
    dt = jl_new_datatype((jl_sym_t*)name, modu, NULL, (jl_svec_t*)para,
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/keno/julia/src/interpreter.c:244:23: note: Value may have been GCed here
    jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1);
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/keno/julia/src/interpreter.c:255:9: note: Argument value may have been GCed
        jl_set_datatype_super(dt, super);
        ^                     ~~

Talking to Jeff and Jameson, these are likely never seen in real code because leaf types are generally rooted
somehow and we don't like collecting them. However, it seems like there's no guarantee that this won't happen
(in contrived) situations, so let's put the root here to be safe.

ClangSA was unhappy about this:
```
/home/keno/julia/src/interpreter.c:239:10: note: Started tracking value here
    dt = jl_new_datatype((jl_sym_t*)name, modu, NULL, (jl_svec_t*)para,
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/keno/julia/src/interpreter.c:244:23: note: Value may have been GCed here
    jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1);
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/keno/julia/src/interpreter.c:255:9: note: Argument value may have been GCed
        jl_set_datatype_super(dt, super);
        ^                     ~~
```
Talking to Jeff and Jameson, these are likely never seen in real code because leaf types are generally rooted
somehow and we don't like collecting them. However, it seems like there's no guarantee that this won't happen
(in contrived) situations, so let's put the root here to be safe.
@yuyichao
Copy link
Contributor

these are likely never seen in real code because leaf types are generally rooted somehow

We are actually heavily (like really heavily) relying on that but these code is exactly how we ensure that so the rooting should be needed before we actually set the binding.

@ararslan ararslan added GC Garbage collector compiler:interpreter labels Dec 16, 2017
@Keno Keno merged commit 8bf4e9d into master Dec 16, 2017
@martinholters martinholters deleted the kf/interpdtroot branch December 16, 2017 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants