NPE inside jitted code in Bundler when bundling Rails 5.0.0.1 #4235
Comments
|
Ok, it looks like this is an IR issue. Here's the method, following by explanation of the error. def search_for(dependency)
platform = dependency.__platform
dependency = dependency.dep unless dependency.is_a? Gem::Dependency
search = @search_for[dependency] ||= begin
index = index_for(dependency)
results = index.search(dependency, @base[dependency.name])
if vertex = @base_dg.vertex_named(dependency.name)
# ***** CONDITIONALLY INITIALIZED *****
locked_requirement = vertex.payload.requirement
end
spec_groups = if results.any?
nested = []
results.each do |spec|
version, specs = nested.last
if version == spec.version
specs << spec
else
nested << [spec.version, [spec]]
end
end
nested.reduce([]) do |groups, (version, specs)|
# ***** NEXT LINE NPEs *****
next groups if locked_requirement && !locked_requirement.satisfied_by?(version)
groups << SpecGroup.new(specs)
end
else
[]
end
# GVP handles major itself, but it's still a bit risky to trust it with it
# until we get it settled with new behavior. For 2.x it can take over all cases.
if @gem_version_promoter.major?
spec_groups
else
@gem_version_promoter.sort_versions(dependency, spec_groups)
end
end
search.select {|sg| sg.for?(platform, @ruby_version) }.each {|sg| sg.activate_platform!(platform) }
endThe I'll try to come up with a small repro. |
|
Well that didn't take long. |
|
Looks like a bug in the null var inits code .. b is probably not being init to null. will take a look later tonight unless you guys get to it first. |
|
The bug is in .. the identifyInits method .. That code is not looking at variables used but not defined in nested closures at all. |
|
As I am getting ready to head out in the morning (in Seattle at some offsites), I thought I would quickly pop in and mention that this is a simple fix but I cannot fix it now since I am going to be out and about the whole day. But here is the quick scoop in case anyone wants to take a crack at it. IRScope used to maintain state about used and defined vars. So, all that needs to be done is for 'identifyMissingInits' code to collect all used-but-not-defined vars from all nested closures (all the way to the leaf) that belong to current scope and add it to the set of vars needing inits. |
|
Okay, I'll take a crack at this now for a short while and get back to it tonight. It might be a bit more involved than the outline above if we don't want to be extra conservative around closures. |
|
This functionality needs a whole bunch of specs with all kinds of corner cases. Can someone create them? I am not going to be able to till next week at least. |
|
@subbuss You're awesome...I was going to look into it this afternoon, but instead I'll try to fill out some edge case tests. |
|
@subbuss What kinds of corner are you thinking of? I thought through various amounts of nesting closures, different contexts like class bodies...none seemed like they'd be very valuable. And more complicated conditional cases reduce logically to the simple case anyway. |
|
I'm either going crazy or this is still broken on master. I tried to clone and bundle https://github.com/floehopper/introspection, and I'm getting the same trace again: This is the same line as before: #4235 (comment) It seems the case used in bundler is still broken, even though we fixed the reduced case. |
|
I have come up with a reproduction. It's a bit more complicated than we thought. The following code is structured to better reproduce the bundler issue. The key events that seem to cause the error are:
If you run this script with Without the "nil" output, here's the lines I get. |
|
@headius @subbuss so when JIT fails we do not full build and run in interp we fall back to startup interp. The closure is assuming it will access the lvar from code with missing inits added. The primary issue being you cannot add inits to startup instructions since it is just a list. We could maybe in JIT failure case retain full build and use full interp since it cannot fail and would have the inits but I am not sure how this fits into how we store references in the *Methods. Can a mixed mode method use either full interp engire OR a JIT compiled method? I guess I will poke around there. |
|
So rough draft and this feels a bit icky:
|
|
In the middle of some parsoid work, but, quick qn: won't the interp continue to use the return nil if null logic in dynamic scope? or did that get removed? |
|
@subbuss My understanding at this point is it is the recompiled nested closure which is making the assumption so it is not interpreting. |
|
Another option would be that since closure needs to know that it is compiling over a failed parent scope perhaps it can emit nil checking? Can we know that? I guess it would be nice to know at IRScope level. |
|
Ok, ignore the bit about the closure jitting twice. The first closure to JIT was the each loop I used to anchor the big array. Here's the script with that loop removed: <nils removed>
def search_for(blah)
asdf = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,]
asdf.to_s
if blah
locked_requirement = "foo"
end
[1..100].each do |s|
p locked_requirement
end
end
1000.times {
search_for(false)
}And the full output: |
|
Further simplification of the script shows that even if ONLY the closure JITs, it blows up. def search_for(blah)
asdf = [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,]
asdf.to_s
if blah
locked_requirement = "foo"
end
100.times do |s|
locked_requirement.to_s
p s
end
end
3.times { search_for(false) }Run like So this is a mismatch of running jitted blocks in any unjitted context that might have uninitialized variables, because the IR interpreters do not make any guarantees that those variables have been initialized. |
|
Ok we have talked quite a bit about this. The issue is whether we fail to JIT method then JIT a closure after that failure OR we execute a child closure enough to JIT before the method we can end up hitting a case where the parent scope has no var init code in it. In the case of a failure of a method we can fall over to use Full interp with the Full code we just made. This has one risk that we seem to no longer run CI against Full interp with all passes. It also cannot solve the second scenario where a block happens to run first. The other solution is that a closure JITting can look at parent scopes up to a hard scope (method/closure/module/script) and if any of them do not have a fullIC we have to emit nil checks in the emitted bytecode. This solution has the downside that hot closures will be penalized and will never get fixed later unless the method happens to JIT. We realized that we can do this lack of nil checking on any hard scopes since they cannot capture another scopes variables. So that is what we plan on keeping for 9.1.6.0. We can also skip checks for any closures which do not capture a parents. Possibly we can check full status of captured parents and still do opt if they have all been compiled (e.g. have fullIC). |
|
Ah another one. any vars local to the closure scope itself need not be nil checked since we will run pass to add missing inits for it. |
This is a "fix" for #4235 in that it prevents closures from ever blindly reading closed-over variables that have not been initialized to nil. This may happen if the block jits independent of the method, in which case the method will continue to be run through null-checking interpreter logic, so the closure code cannot guarantee all values are non-null on first read. This is only a "fix" in quotes because it completely disables the fast path optimizaton for all non-method JIT. Note that I did add specialized "OrNil" paths to the generated DynamicScope subclasses, so that should at least inline to a field plus null check.
|
I have done an emergency "fix" (0e65638) for this that disables the fast path scope gets if we're not in a method body. This avoids the problem of a parent scope running in interpreter (with null checks and without initialization logic) containing a closure that has been jitted (without null checks). I also generated the additional specialized "OrNil" methods into our DynamicScope subtypes, so the null-checking paths should inline down to a field get, a null check, and a possible field set. This should be a good enough fix for 9.1.6.0 but as @enebo says we're penalizing jitted closures inside scopes that themselves never jit. |
|
Okay, yes, the problem is that as the comment in the analysis shows, it assumes that variables in external scopes have been defined already -- this assumption is violated when a closure is JIT-ted independent of the parent scope. Can this scheme work?
|
|
@subbuss I think just calling up parents to non-closure scope and we can ask isFull or make a method which just notices whether full build has occurred. A full build should add add init as a default pass I think as a base condition to being correct too. I think we just need to assume we have no missing inits before any other flow-based optimizations. As far as base assumptions for state of our instrs missing inits seems like it should be front and foremost to me. |
|
So we have something which is no longer broken but it is non-optimal. I am bumping this one version so we can keep track of doing a better fix. I would make a new issue but since we have not had nil init opto in place in a release yet this would be confusing to list in our issues resolved list anyways. |
|
@enebo Perhaps we should bump to 9.2 since we're starting to wind down the 9.1 series a bit and want to get a 9.1.7.0 out in the next week? |
|
Closing this in favor of another issue. What we have is still less than ideal, since an interpreted method that's only called once will never see a contained block jit, no matter how many times it is called. However the original reported problem was solved, and this is now getting pretty stale. |
Environment
JRuby master as of today.
Rails 5.0.0.1 tag on github.
Bundler 1.13.5.
Expected Behavior
Rails should bundle properly.
Actual Behavior
I get an NPE when bundling. It happens within jitted code and does not appear to affect the simple interpreter. Here's the relevant context; I suspect it is an uninitialized variable coming from our DynamicScope:
The text was updated successfully, but these errors were encountered: