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

Also serialize dt->instance. Fixes #5129 #5149

Merged
merged 1 commit into from
Dec 15, 2013
Merged

Also serialize dt->instance. Fixes #5129 #5149

merged 1 commit into from
Dec 15, 2013

Conversation

Keno
Copy link
Member

@Keno Keno commented Dec 14, 2013

dt->instance is one of the jl_value_t*'s that can occur purely in code
and not anywhere else in the AST. All other instances of where this
happends are captured by li->roots, since they need GC roots that way,
but this instance is rooted by the owning type.

@JeffBezanson @vtjnash

@JeffBezanson
Copy link
Member

I don't think this is the full fix. What we want to do is iterate over the jl_value_to_llvm table and dump all the needed indexes. This would subsume jl_serialize_gv_syms and all the other ad-hoc calls to jl_serialize_gv. The only thing that makes this difficult is that some entries are bindings, which are not ordinary values. However, bindings deserve a different treatment anyway (#5125).

@Keno
Copy link
Member Author

Keno commented Dec 14, 2013

I can't think of any other instance where this might happen. The reason I didn't instead iterate of the jl_value_to_llvm is that I thought the iteration over backref was there in the first place to avoid serializing values that would not be referenced in the final system image in order to keep the size of the sysimg down if possible.

@JeffBezanson
Copy link
Member

I guess that is handled by jl_serialize_globalvals.

@Keno
Copy link
Member Author

Keno commented Dec 14, 2013

Correct.

@Keno
Copy link
Member Author

Keno commented Dec 14, 2013

Updated to remove unnecessary changes.

@vtjnash
Copy link
Member

vtjnash commented Dec 14, 2013

I actually want to go with Jeff's idea in the long term. Anything in jl_value_to_llvm was JIT into the code somewhere, so we should make sure it has a reference.

However, as Jeff pointed out, this will require special handing of jl_binding_t objects, but that's needs to be fixed soon anyways. (It may also require some special handling of function pointers, but that's not really an issue either).

For now however, this is probably fine.

all the other ad-hoc calls to jl_serialize_gv

Those are actually already gone. My last commit before the branch was merged moved all of the serialization of global variable stuff to the end of the file.

@Keno
Copy link
Member Author

Keno commented Dec 14, 2013

Wouldn't the better solution there be to cut out the unreferenced IR rather than including unreferenced julia objects?

@Keno
Copy link
Member Author

Keno commented Dec 14, 2013

Third time's the charm.

@JeffBezanson
Copy link
Member

It doesn't look like you actually return the placeholder value anywhere.

dt->instance is one of the jl_value_t*'s that can occur purely in code
and not anywhere else in the AST. All other instances of where this
happends are captured by li->roots, since they need GC roots that way,
but this instance is rooted by the owning type.
@Keno
Copy link
Member Author

Keno commented Dec 15, 2013

Doh! Fixed.

JeffBezanson added a commit that referenced this pull request Dec 15, 2013
Also serialize dt->instance. Fixes #5129
@JeffBezanson JeffBezanson merged commit bb54883 into master Dec 15, 2013
@Keno Keno deleted the kf/5129 branch August 10, 2014 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants