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

fix five debugger issues #3561

Merged
merged 5 commits into from Sep 7, 2017
Merged

fix five debugger issues #3561

merged 5 commits into from Sep 7, 2017

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Sep 7, 2017

This fixes three debugging issues

There were two other issues fixed along the way

  • The body of a let rec did not have a sequence point placed. That is fixed.
  • When partial applications were made, such as printfn "..." a b , the allocation of the format string was done prior to the printf. That could cause odd step-through behaviour.

The changes are

Testing is mostly through codegen tests - nearly all will need to be updated because along the way a nop instruction has disappeared from the top of most methods, because of the fix to #92 . I've also updated TheBigFileOfDebugStepping.fsx with new examples and manually done step-through for the whole file.

@cartermp
Copy link
Contributor

cartermp commented Sep 7, 2017

Oh wow, this is great!

@dsyme dsyme changed the title fix three debugger bugs fix five debugger bugs Sep 7, 2017
@dsyme dsyme changed the title fix five debugger bugs fix five debugger issues Sep 7, 2017
@dsyme
Copy link
Contributor Author

dsyme commented Sep 7, 2017

@cartermp You (or someone else) might want to step the TheBigFileOfDebugStepping.fsx on .NET Core, just to check things are ok. Also in VSCode/Ionide.

I compile using release\net40\bin\fsc.exe --optimize- --debug+ TheBigFileOfDebugStepping.fsx then run devenv /debugexe TheBigFileOfDebugStepping.exe

@dsyme
Copy link
Contributor Author

dsyme commented Sep 7, 2017

A horrible number of test baselines needed updating about 150 files

@cartermp
Copy link
Contributor

cartermp commented Sep 7, 2017

Ugh, I can't test anything. Nothing will build on my machine with { 15.3.3, VSSDK, Win10 SDK, F# support }, after a git clean -xdf even.

C:\repos\visualfsharp\packages\Microsoft.VSSDK.BuildTools.15.0.26201\tools\vssdk\Microsoft.VsSDK.targets(966,5): error : CreatePkgDef : error : ReflectionTypeLoadException: Unable to load one or more of the requested types. Retrieve t
he LoaderExceptions property for more information. [C:\repos\visualfsharp\vsintegration\src\FSharp.LanguageService\FSharp.LanguageService.fsproj]

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice ... you've been busy.

@cartermp
Copy link
Contributor

cartermp commented Sep 7, 2017

Okay, I got it to build after uninstalling nightlies and using VS command prompt in admin mode. Not sure which of that fixed it. Argh. Anyways, I can test this this week

@dsyme
Copy link
Contributor Author

dsyme commented Sep 7, 2017

@dotnet-bot test this

@dsyme
Copy link
Contributor Author

dsyme commented Sep 7, 2017

Well, I updated about 250 code generation test files (and simplified the naming of some others).

I took a look through as many of these as I could and basically all seemed to be the 'nop' removal. For example this one clearly has had one opode removed.

It's possible that something is being hidden by such a large test update. But I think it's still ready for merge, there's not a l;ot more we can do here besides staring at the diffs and manual testing.

@dsyme
Copy link
Contributor Author

dsyme commented Sep 7, 2017

@KevinRansom I think we may as well merge this - review welcome for the src\fsharp files

@KevinRansom
Copy link
Member

I already looked, I forgot to approve ... sorry.

@dsyme
Copy link
Contributor Author

dsyme commented Sep 8, 2017

@KevinRansom You did approve, I just didn't notice it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants