This takes Steffens repro and has a slightly different fix -- Fixes I1556

[KevinRansom]

The Desktop PDB Writer (--debug:full) --- creates a nested pdb scope for each new line in the same function scope.

The pdbwriter stack overflows when there is a very deeply nested set of pdb scopes.

This fix changes the fsharp compiler behavior to not create a new nested scope if the new scope has the sames dimensions as it's parent scope.

This fix is only for full and pdbonly builds. Consider making a similar optimization for portable pdbs.

Tested with a file with 6000 let bindings. The compiler does fine, however, the debugger has a meltdown :-)

Kevin

[KevinRansom]

Note: Use merge-pull request to correctly assign contributions.

[forki]

The question is if we need both fixes. Merging scopes will allow more bindings and the explicit limit will safe compiler from crashing

[KevinRansom]

@dotnet-bot test this please

[KevinRansom]

So .. a bunch of failures are due to changes in the .line directive emitted into .il so now I have the joy of figuring out whether that has an experience impact.

[KevinRansom]

@forki

I doubt if anyone will be able to nest 450 scopes without a code generator. And my guess is ... the debugger itself will have a hard time with it, timing out . Also ... this is a bug with a trivial repro that would seem like it should be very common and we haven't seen it before.
The other thing is this is for desktop pdbs and they are for sure legacy ... the advantages of portable pdbs are immense so I expect this code path to be used less and less over time.

Kevin

[KevinRansom]

Just for fun I made a file that would nest 500 scopes deep ...
it looks like this:

foo2.txt

If anyone has a file like this ... I think a stack overflow would be the least of their problems.

Kevin

[forki]

can you still put it in? We would still assume the compiler doesn't crash on such a file, right?

[KevinRansom]

@forki

The depth is actually dependent on the complexity of each nested level. To test this I used this code to generate a sample source file:

let lines depth complexity =
    let indent n = "".PadRight(n * 2)
    seq {
        for i = 0 to depth do
            yield indent i + "let a =\n"
            for j = 1 to complexity do
                yield (indent (i + 1)) + "let a = 10\n"
        for i = depth downto 0 do
            yield indent (i + 1) + "10\n"
    }

lines 300 100 |>Seq.iter(fun l -> printf "%s" l)

This creates a file that has 300 nested lets, each one of which contains 100 lets then returns a constant.

Both of these numbers are below the 450 limit,

When I used the code from the original pr, which limits scope creation to less than 450 the stack overflowed during build.

Given that limiting the maximum number of scope levels doesn't ensure that stack overflows are avoided it seems like the benefit is not really that usefull. Also the limiting approach would mess with the debugging experience so perhaps we are just pushing the problem elsewhere. I have to say I prefer the build to fail, than the debugger to do wierd things.

Am I crazy?

Kevin

              // Write the scopes 
              let rec writePdbScope level sco = 
                  if level = 0 || (level < 450 && (sco.Locals.Length <> 0 || sco.Children.Length <> 0)) then 
                      pdbOpenScope !pdbw sco.StartOffset
                      sco.Locals |> Array.iter (fun v -> pdbDefineLocalVariable !pdbw v.Name v.Signature v.Index)
                      sco.Children |> Array.iter (writePdbScope (level + 1))
                      pdbCloseScope !pdbw sco.EndOffset
              writePdbScope 0 minfo.RootScope

[smoothdeveloper]

As I initially hit this issue, it was quite terrifying to have fsc do stack overflow exception without any indication.

Having a clearer error message or stack trace would be better, but I'm currently facing this issue with generated code, I'd say having it compile in DEBUG even if I can't step properly everywhere in it is still better than fsc crashing on me, at least for my use case.

I have disabled DEBUG but it impacts the whole assembly and I'd still like to debug the rest of my code there.

[KevinRansom]

@smoothdeveloper ...

your code will correctly compile and debug with the fix in this PR (this fix is based on merging parent and child scopes with the same extents) ... indeed I am quite surprised that someone didn't find this before given that the repro is simply a file with a moderate number of functions defined. I have to assume that most developers clump functions into modules and types and thus avoid a large number of functions.

The issue now is this ... will counting the nested scope level allow us to avoid this bug ... The fix where we counted to 450 and stopped creating scopes also eliminated the problem you saw ... however ... I was able to relatively easily create a different repo that still stack overflowed despite having the scope level max set to 450 and using nesting considerably below that limit.

Given that your scenario is eliminated by scope merging and that should drastically reduce the likelihood of anyone encountering this bug in the future and that counting scope levels does not really solve the problem because there appears to be a complexity component to the stack use. I do not propose to implement overflow protection based on an arbitrary number of scopes.

[smoothdeveloper]

@KevinRansom I just looked at your file and now I understand more "I think a stack overflow would be the least of their problems." which I confused with the initial reproduce, so it sounds we are good for the "sane" use case now.

Is there anything which can be done to avoid encountering the raw stack overflow happening when calling into that external component? or would that be too invasive or not deterministic enough to warrant the effort?

[KevinRansom]

@smoothdeveloper

There's not much that we can really do that will reliably work ... also it is the Windows pdbwriter ... I expect that we will drop support for generating full pdb's in a couple of releases, the latest release of VS2015 supports debugging using portable pdbs. They are much smaller than old style pdbs, we can embed them, and they can carry source code too, and most importantly ... they are cross platform.

Kevin

[smoothdeveloper]

Thanks for further context, I can connect the dots with other comments you left now, good to know that new VS supports the new pdb system and that this issue will vanish then.

What will VS users need to do to switch their F# projects to the new .pdb system? will it work on non-core .net framework?

[KevinRansom]

@smoothdeveloper
Portable pdbs work on the full desktop as well as the coreclr. The --debug: commandline option has been extended with portable and embedded to generate portable pdbs and embedded portable pdbs. We still need to add embedding source in pdbs and fixing all of the bugs. I'm not sure if we will have an option in the UI to enable it yet, but eventually it will just be a UI option.

Kevin

[forki]

Thanks Kevin for your analysis. So basically we can't predict when that
component will die, so counting levels is only helpful in one case. That's
sad.

OK one last question: do you know if the C# compiler is doing it like we do
it? Maybe they use a different version of the same thing that would not
crash.

Am 20.09.2016 12:40 vorm. schrieb "Kevin Ransom (msft)" <
notifications@github.com>:

@smoothdeveloper https://github.com/smoothdeveloper
Portable pdbs work on the full desktop as well as the coreclr. The
--debug: commandline option has been extended with portable and embedded to
generate portable pdbs and embedded portable pdbs. We still need to add
embedding source in pdbs and fixing all of the bugs. I'm not sure if we
will have an option in the UI to enable it yet, but eventually it will just
be a UI option.

Kevin

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1545 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNIPccTuLt5uJUBMohCYqBQRDOgVKks5qrw8_gaJpZM4J_fio
.

[dsyme]

+1 LGTM

[KevinRansom]

@forki I would expect the C# compiler to crash also scopes nest by design and are used to collect local local variables together that share the same visibility extents. A nesting that deep is probably not really expected to occur in real life code.

Kevin

[KevinRansom]

Merging.