Huge literal in SurfaceArea.net40.fs causes mono to crash

[NatElkins]

Mono gets a stack overflow when trying to run FSharp.Compiler.Service with that enormous file (https://github.com/Microsoft/visualfsharp/blob/44c7e10ca432d8f245a6d8f8e0ec19ca8c72edaf/tests/FSharp.Core.UnitTests/SurfaceArea.net40.fs). Can I throw that literal in a text file and change the test to read it into memory instead?

Because of this issue I can't open FSharp.sln with Rider (I have other issues getting the solution to work with Rider also, but this is the big showstopper right now).

Repro steps

I initially reported it here: JetBrains/resharper-fsharp#23

@auduchinok did more investigation here: mono/mono#11933

Expected behavior

Shouldn't crash.

Actual behavior

Crashes.

Known workarounds

Splitting the file into two literals allows Rider to open the solution, or reading it into memory from a text file.

[cartermp]

I personally wouldn't mind it but we do have a precedent of just having string literals in code for tests; this would also imply an update to the test guide so people know to update the file

[NatElkins]

Cool.

Why is an extra note required to the test guide? I don't see anything specific about updating the string literals. Won't the test just fail if it's not updated, same as if the literal was not updated?

[cartermp]

In this case it's a little more difficult to deal with an additional file.

[cartermp]

But it's a bit concerning that mono can't handle a string literal like this, is the bug filed on them yet?

[baronfel]

Yeah. The part that's especially concerning here is that I see similar behavior on netcore FSAC for largish (~3k lines) files: mono/mono#11933 (comment)

[dsyme]

It does feel like tailcalls aren't being taken where we expect them to be, ugh.

The relevant file is https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/lex.fsl.

The difference in IDE tooling is that skip is false, meaning whitespace and end-of-line are reported, including line by line in very long strings, and a continuation is used to restart it.

One way to fix this specific issue by using less stack is to skip over the string in larger chunks, e.g. replace all of https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/lex.fsl#L835-L864 by

 | eof 
    { let (_buf,_fin,m,args) = sargs 
      EOF (LexCont.VerbatimString(!args.ifdefStack,m)) }
 | surrogateChar surrogateChar // surrogate code points always come in pairs            
 | (('\Lu' | '\Ll' | '\Lt' | '\Lm' | '\Lo' | '\Nl' | '\Nd' | anywhite) | _) +
    { let (buf,_fin,m,args) = sargs 
      addUnicodeString buf (lexeme lexbuf) 
      if not skip then (STRING_TEXT (LexCont.VerbatimString(!args.ifdefStack,m))) else verbatimString sargs skip lexbuf }

The crucial line is this:

 | (('\Lu' | '\Ll' | '\Lt' | '\Lm' | '\Lo' | '\Nl' | '\Nd' | anywhite) | _) +

which I believe says "consume all letters, digits or whitespace as a single chunk". If tailcalls aren't being taken the above will still grow the stack one frame per line in a verbatim string (it is currently growing one frame per identifier or line). That should scale much better

But the fundamental problem of some tailcalls not being taken - and also x64 using more stack - is concerning.

[dsyme]

I've looked at this again. The problem isn't quite what I thought it was.

First, clearly direct mutually recursive tailcalls are not being taken in some situations by .NET Core and Mono. This is a concern and I'm surprised we haven't hit this problem before.

However, we shouldn't be exposed to this problem here. The generated lexer has this kind of code:

       let rec ident arg lexbuf = _fslex_ident arg 10 lexbuf

       and verbatimString arg lexbuf = _fslex_verbatimString arg 15 lexbuf

       and _fslex_ident arg state lexbuf = 
           match ... state .. with... 
               ... recursive calls to ident ....

       and _fslex_verbatimString arg state lexbuf = 
           match ... state .. with... 
               ... recursive calls to verbatimString ....

The problem here is that the recursive calls aren't direct tailcalls and thus we expose ourselves to problems with the .NET CLR tailcall implementation more than I thought when lexing. Fortunately we can easily change the generated code to eliminate the _fslex_... funtions which are only there to record the initial state at each entry point, e.g.

       let rec ident arg lexbuf =  ... recursive calls to ident ....
           match ... 10 .. with... 
               ... recursive calls to ident ....

       and verbatimString arg lexbuf = 
           match ... 15 .. with... 
               ... recursive calls to verbatimString ....

This code is actually clearer and cleaner and faster. I'm fixing this in fsprojects/FsLexYacc#89 We now have to update this repo to use the updated FsLexYacc.

[baronfel]

@dsyme for reference how did you inspect the generated lexer? Looking at build output to see where the outputs of the fslex generation was?

[dsyme]

@dsyme for reference how did you inspect the generated lexer? Looking at build output to see where the outputs of the fslex generation was?

Yes, just looked at fslexlex.fs