-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Stack Overflow when using curried interface member, no stack overflow with tupled version #87393
Comments
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsDescriptionThe following program causes a stack overflow in release build. If you follow the comments and use the tupled version of namespace Bar
type Foo<'a> =
| Pure of 'a
| Apply of ApplyCrate<'a>
// This curried version of "ApplyEval" causes a stack overflow in Release compilation
and ApplyEval<'a, 'ret> = abstract Eval<'b,'c,'d> : 'b Foo -> 'c Foo -> 'd Foo -> ('b -> 'c -> 'd -> 'a) Foo -> 'ret
// This tupled version of "ApplyEval" does not cause a stack overflow in Release compilation
// and ApplyEval<'a, 'ret> = abstract Eval<'b,'c,'d> : ('b Foo * 'c Foo * 'd Foo * ('b -> 'c -> 'd -> 'a) Foo) -> 'ret
and ApplyCrate<'a> = abstract Apply : ApplyEval<'a, 'ret> -> 'ret
module Foo =
let apply2 (b : Foo<'b>) (c : Foo<'c>) (d : Foo<'d>) (f : Foo<'b -> 'c -> 'd -> 'a>) : Foo<'a> =
{ new ApplyCrate<'a> with
member _.Apply<'ret> (eval : ApplyEval<'a,'ret>) =
// eval.Eval(b, c, d, f) // uncomment for tupled version
eval.Eval b c d f // uncomment for curried version
}
|> Apply
let lift a = Pure a
let rec evaluateCps<'a, 'b> (f : 'a Foo) (cont : 'a -> 'b) : 'b =
match f with
| Pure a -> cont a
| Apply crate ->
crate.Apply
{ new ApplyEval<_,_> with
// member _.Eval((b, c, d, f)) = // uncomment for tupled version
member _.Eval b c d f = // uncomment for curried version
evaluateCps f (fun f ->
evaluateCps b (fun b ->
evaluateCps c (fun c ->
evaluateCps d (fun d -> cont (f b c d))
)
)
)
}
module TailRecBug =
let overflowtest () =
let rec makeChain (height : int) (k : int Foo -> 'r) : 'r =
if height <= 0 then
k (Foo.lift 0)
else
makeChain (height - 1)
(fun b ->
let c = Foo.lift height
let f = Foo.lift (fun a b c -> a + b + c)
Foo.apply2 b c c f |> k)
let chain = makeChain 10000 id
Foo.evaluateCps chain id
module Main =
[<EntryPoint>]
let main _argv =
let r = TailRecBug.overflowtest()
printfn "%A" r
0
IL of curried version
IL of tupled version
Reproduction Steps
Expected behaviorThe runtime obeys the Actual behaviorA stack overflow happens:
Regression?As far as I know, this is not a regression. Known WorkaroundsUse the tupled version Configuration.NET 7 on x64 Win11 Other informationNo response
|
Thanks for the report. The problem here is that the JIT gets confused because internally, the call to |
Wow, thanks for the super fast feedback. Much appreciated. |
…esolving In the helper-based tailcall mechanism it is possible that we expand the target call into two actual calls: first, a call to CORINFO_HELP_VIRTUAL_FUNC_PTR to compute the target, and second a call to that target. We were not taking into account that the return address needed for the tailcall mechanism needs to be from the second call. In this particular case the runtime does not request the JIT to pass the target; that means we end up resolving the target from both the caller and from the CallTailCallTarget stub. Ideally the JIT would be able to eliminate the CORINFO_HELP_VIRTUAL_FUNC_PTR call in the caller since it turns out to be unused, but that requires changes in DCE (and is somewhat non-trivial, as we have to preserve a null-check). A simpler way to improve the case is to just change the runtime to always request the target from the JIT for GVMs, which means the CallTailCallTarget stub no longer needs to resolve it. That also has the effect of fixing the original issue, but I have left the original fix in as well. Fix dotnet#87393
Awesome, thank you so much! |
…esolving (#87395) In the helper-based tailcall mechanism it is possible that we expand the target call into two actual calls: first, a call to CORINFO_HELP_VIRTUAL_FUNC_PTR to compute the target, and second a call to that target. We were not taking into account that the return address needed for the tailcall mechanism needs to be from the second call. In this particular case the runtime does not request the JIT to pass the target; that means we end up resolving the target from both the caller and from the CallTailCallTarget stub. Ideally the JIT would be able to eliminate the CORINFO_HELP_VIRTUAL_FUNC_PTR call in the caller since it turns out to be unused, but that requires changes in DCE (and is somewhat non-trivial, as we have to preserve a null-check). A simpler way to improve the case is to just change the runtime to always request the target from the JIT for GVMs, which means the CallTailCallTarget stub no longer needs to resolve it. That also has the effect of fixing the original issue, but I have left the original fix in as well. Fix #87393
Hey @jakobbotsch, any chance to get this backported to .NET 7? That would help a very big F# customer ;) Also, is it possible to predict when the nightlies will have it? |
Yes, we can backport this if the workaround is not sufficient for your situation. Is it the case? cc @JulieLeeMSFT Note that the workaround may actually have better performance characteristics. It trades off an allocation for being able to make a more efficient tailcall (basically a single
I can see that the current nightly is built from 54dab73 (you can click the blue version badge in the table in dotnet/installer to get this). Presumably the next build should have the fix, but I'm not sure when it will be available. |
I think, it would be good to have in 7 to lower the risk of running into it, as one might not see the StackOverflow during development and moving production code to a new 7 release is done quicker than to 8. |
Alright, I can confirm it's fixed in |
Glad to hear it.
We typically only backport fixes when there is a concrete user scenario, not to minimize potential risk (since the fix itself comes with risk, the bar for servicing older releases is higher). In this case the behavior is quite old (from .NET core 3.1, I believe) and requires a specific set of circumstances to trigger:
Based on this another workaround would be to store the target into a delegate and then do the call through that. It has the benefit that it does not require changing the shape of the API. E.g. add the following function: [<MethodImpl(MethodImplOptions.NoInlining)>]
let applyEval<'a, 'b, 'c, 'd, 'e> (fn : 'a -> 'b -> 'c -> 'd -> 'e) b c d f =
fn b c d f and change With that said, if the workaround is not sufficient (or hard to apply) then I would be happy to backport the fix -- please let me know! |
I talked with the affected party, it's enough to have this in .NET 8 :) |
Description
The following program causes a stack overflow in release build. If you follow the comments and use the tupled version of
ApplyEval
the stack overflow doesn't happen and the program runs till exit.As far as I understand the dump of the IL, there are
tail.
prefixes in both versions but it seems that the runtime doesn't obey them for the curried version.In the diff between the curried and the tupled IL, I see a big differences in the stack of
.override method instance !1 class Bar.ApplyEval`2<!a,!b>::Eval<[3]>
Maybe that's a hint that can help?
Please tell me if I can help in any way or if this should be raised in another repository.
IL of curried version
IL of tupled version
stackoverflow.zip
Reproduction Steps
compile the provided .fs file in Release mode
run the exe
stackoverflow happens
follow the comments to switch to the tupled version
compile the provided .fs file in Release mode
run the exe
stackoverflow doesn't happen
Expected behavior
The runtime obeys the
tail.
prefix for both version and no stack overflow happens in Release modeActual behavior
A stack overflow happens:
Regression?
As far as I know, this is not a regression.
Known Workarounds
Use the tupled version
Configuration
.NET 7 on x64 Win11
Other information
No response
The text was updated successfully, but these errors were encountered: