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

Fatal error in type inference adding a GC frame #12603

Closed
mbauman opened this issue Aug 13, 2015 · 14 comments
Closed

Fatal error in type inference adding a GC frame #12603

mbauman opened this issue Aug 13, 2015 · 14 comments
Labels
needs tests Unit tests are required for this change

Comments

@mbauman
Copy link
Member

mbauman commented Aug 13, 2015

Version: 0.4.0-dev+6494 e66175b

Minimal example:

@inline function f(A::AbstractArray, I1, I2, I3)
    (size(A,1) >= I1 && size(A,2) >= I2) && size(A,3) >= I3
end
h(A) = f(A, 1, 2, 3)

code_typed(f, (Array{Int, 3}, Int, Int, Int)) # is ok
code_typed(h, (Array{Int, 3},)) # has an unreachable error("fatal error in type inference")
code_llvm(f, (Array{Int, 3}, Int, Int, Int)) # no gc frame
code_llvm(h, (Array{Int, 3},)) # gc frame (with nothing put into it, I think)

Full output: https://gist.github.com/mbauman/a4e9fb36f67e2287a8f6

@mbauman
Copy link
Member Author

mbauman commented Aug 13, 2015

It's not that big of a deal to have assertion checks like that, especially when LLVM can verify that things were ok and drop the dead code. But perhaps we could make it a non-allocating error type instead to prevent GC frames?

@yuyichao
Copy link
Contributor

I would still prefer not having to worry about gc frames when an error is thrown. I think my throw local gcframe branch can already handle this and I believe when the codegen rewrite is merged there's even more we can do about it.

Any idea why type inference decide to add an assertion?

@mbauman
Copy link
Member Author

mbauman commented Aug 13, 2015

Agreed completely, but this is blocking #11895. No clue as to what's going on, but this is about as simple of an example as it gets, so hopefully one of you inference gurus can figure it out.

@yuyichao
Copy link
Contributor

Hmmm throw(ErrorException("....")) should not create a gc frame and code_llvm agrees. However, there's something wrong with the codegen of error(...). It seems that type inference could not infer that Main.Base is statically known?

@yuyichao
Copy link
Contributor

Maybe you can try to have different definitions of error for base and inference?

There also seems to be a missing Main.Base. for systemerror (although I'm not sure if it is used in inference)...

@yuyichao
Copy link
Contributor

Or replace Expr(:call,:error,"fatal error in type inference") with Expr(:call, :throw, Expr(:new, :ErrorException, "fatal error in type inference")) just for testing.

@mbauman
Copy link
Member Author

mbauman commented Aug 13, 2015

Thanks for the quick hack!

Inference can't figure it out because Main isn't a constant binding. Why is error.jl different from all the other tools inference uses (which get copied)?

@yuyichao
Copy link
Contributor

Other tools?

@mbauman
Copy link
Member Author

mbauman commented Aug 13, 2015

All the other stuff in coreimg.jl. My knowledge here is rudimentary, but aren't all those functions and types duplicated and frozen into Core.Inference? They don't reach out and try to grab the current methods and types from Main like error.jl is doing.

@yuyichao
Copy link
Contributor

I won't claim I know the bootstrap very well but it does seems a little bit strange that we pull error() but not ErrorException into Core.Inference....

@JeffBezanson
Copy link
Member

We should just remove that. Internal compiler errors shouldn't be emitted into generated code.

@carnaval
Copy link
Contributor

yep. maybe emit it in debug mode and only have it be an unreachable llvm op in release.

@mbauman
Copy link
Member Author

mbauman commented Aug 6, 2016

The original testcase here now passes… and the fatal error line has changed to use TopNode/GlobalRef in fba99fd which I think solves the root issue.

@mbauman mbauman closed this as completed Aug 6, 2016
@tkelman
Copy link
Contributor

tkelman commented Aug 7, 2016

Do we have test cases that capture this running as part of the test suite?

@tkelman tkelman added the needs tests Unit tests are required for this change label Aug 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Unit tests are required for this change
Projects
None yet
Development

No branches or pull requests

5 participants