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 missing GC root in interpreter.c #28742

Merged
merged 2 commits into from
Aug 18, 2018
Merged

Fix missing GC root in interpreter.c #28742

merged 2 commits into from
Aug 18, 2018

Conversation

Keno
Copy link
Member

@Keno Keno commented Aug 18, 2018

Static analysis complains in relevant part:

/home/keno/julia-1.0/src/interpreter.c:154:9: warning: Argument value may have been GCed
        jl_set_datatype_super(dt, super);
        ^
/home/keno/julia-1.0/src/interpreter.c:144:10: note: Started tracking value here
    dt = jl_new_abstracttype(name, modu, NULL, (jl_svec_t*)para);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/keno/julia-1.0/src/interpreter.c:146: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-1.0/src/interpreter.c:154:9: note: Argument value may have been GCed
        jl_set_datatype_super(dt, super);
        ^                     ~~

Seems true to me. Add dt to the roots in this function. I thought we had already done this and indeed we covered similar cases in #25122, but this one seems to have been forgotten.

Static analysis complains in relevant part:
```
/home/keno/julia-1.0/src/interpreter.c:154:9: warning: Argument value may have been GCed
        jl_set_datatype_super(dt, super);
        ^
/home/keno/julia-1.0/src/interpreter.c:144:10: note: Started tracking value here
    dt = jl_new_abstracttype(name, modu, NULL, (jl_svec_t*)para);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/keno/julia-1.0/src/interpreter.c:146: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-1.0/src/interpreter.c:154:9: note: Argument value may have been GCed
        jl_set_datatype_super(dt, super);
        ^                     ~~
```
Seems true to me. Add `dt` to the roots in this function.
@yuyichao
Copy link
Contributor

Seems valid.
Seems that moving the jl_get_binding_wr up is another possible fix?

@Keno
Copy link
Member Author

Keno commented Aug 18, 2018

Seems that moving the jl_get_binding_wr up is another possible fix?

Yeah, but try explaining how that rooting works to the static analyzer ;)

@Keno
Copy link
Member Author

Keno commented Aug 18, 2018

Also, I just pushed another fix here in the same file.

@yuyichao
Copy link
Contributor

try explaining how that rooting works to the static analyzer

already explained it to you? ;-p

just pushed another fix here in the same file.

Looks good.

@Keno Keno merged commit 54bc9ee into master Aug 18, 2018
@vchuravy vchuravy deleted the kf/interpmissing branch August 18, 2018 22:00
@KristofferC KristofferC mentioned this pull request Aug 19, 2018
@KristofferC KristofferC added bugfix This change fixes an existing bug and removed backport pending 1.0 labels Sep 27, 2018
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants