Turn on CheckForOverflowUnderflow on all projects in Debug

[nguerrera]

Right now, only System.Reflection.Metadata has CheckForOverflowUnderflow on in Debug.

I think there's value in doing this across the board. Expressions that are allowed to overflow by design should be explicitly marked unchecked.

cc @weshaggard @stephentoub

[weshaggard]

Sounds like a good idea.

[mellinoe]

@weshaggard Do you still think this is something we should enable? If we enable this across the board and don't see any errors, is it something we should just go ahead and do?

[stephentoub]

If we enable this across the board and don't see any errors

I'm 100% sure there will be errors / regressions if/when we enable this everywhere. ;) That's not to say we shouldn't do it, but I don't think it's as simple as changing a build configuration entry.

[weshaggard]

I do think this is probably worth doing, but I agree that it is likely not as simple as flipping a build configuration on.

[mellinoe]

Yes I spoke too soon 😄 . We'd definitely need a lot of work to enable it globally. Perhaps it's smarter to start with a few high-value projects and see how much work it is.

[karelz]

Next steps: Flip individual projects and mark code which overflows on purpose as unchecked.

[stephentoub]

Next steps

And measure critical paths to ensure nothing regresses perf-wise (unless we're only flipping the switch for debug, in which case it doesn't matter).

[nguerrera]

(unless we're only flipping the switch for debug, in which case it doesn't matter).

I opened this suggesting debug-only. I'd still recommend starting with that.

[ghost]

I could take this one. Is it sufficient to run the outerloop tests after turning checks on for a project or is there anything else I should try?

[mellinoe]

@dennisdietrich I think that would be a good start. Like @nguerrera said, it's probably a good idea to enable this for Debug builds only, at first. In order to do that, you can condition the property on "'$(ConfigurationGroup)' == 'Debug'" in the project file.

[ghost]

@mellinoe, definitely. Performance aside, turning this on for Release obviously could introduce breaking changes in the form of unexpected exceptions being thrown. Very well, please assign to me.

[ghost]

@mellinoe After thinking about this some more I don't think going for the individual project files is the right approach. We do want this for all projects so it makes more sense to have this in a props file used by all projects. I've noticed that there's already a whole bunch of them so I'll first check if any of those would be suitable (pointers welcome ;)).

[JonHanna]

If you do it across all projects, it's very likely that there'd be a very large number of regressions, so you'd have to change all of those regressions, so it would likely be a massive one-shot PR. If you do it project by project it's more likely to lead to small easier-to-review PRs, and then the setting could be moved up to being global after that.

[karelz]

For the sake of reviewers: Please do NOT do large PRs, break them up :)

[ghost]

@JonHanna Fair enough. @karelz Also fair enough. Though, was exactly is considered 'large' in the corefx team? :)

[karelz]

Imagine you doing the code review: For how long do you want to scroll down and review code?

Here's recent example of rather larger-ish PR: dotnet/corefx#15597. I would personally break it down into smaller chunks - it is easier to review each chunk in 1 session, rather than to remember where you last left off. If the chunk is large ... you are forced to push through the review all at once.
Just my 2 cents.

[ghost]

Suspending further work on this until @stephentoub 's concerns raised in RP dotnet/corefx#15690 are addressed.

[danmoseley]

Another reason piecewise change can be nice is (if we merge without squashing) we can selectively revert or bisect if there's an issue that emerges.

[jkotas]

FWIW, turning on overflow checks in debug builds by default was considered several times in full .NET Framework. It was never done because of it did not have positive ROI. It never found enough bugs to pay for its cost. There is a price you pay for diverging behavior of debug and release build, maintaining annotations in large body of the code, and dealing with bugs introduced by maintaining the annotations.

Having said that, doing experimental runs with overflow checks on and adding some annotations for the heavy hitters should be ok.

[jkotas]

measure critical paths to ensure nothing regresses perf-wise

I am pretty sure that turning this on for release builds is non-starter.

[ghost]

Quick update: Including some code changes I haven't committed yet I'm down to about 50 failing inner loop test projects on Windows so it'll probably be a few more days until I'm done with that, after which I'll take a look at running inner loop on Linux (and after that, obviously, I'll check outer loop).

[tarekgh]

thanks @dennisdietrich this is awesome!

[ghost]

Just noticed some UAP PAL source files. Is that an environment I should check too? If so, how? The building and debugging instructions for Windows don't seem to mention UAP.

[karelz]

We are just bringing UAP back to life, I think it is fine to skip it for now cc: @joperezr @weshaggard.

[ghost]

@karelz Okay. What about endianness? I believe that ARM is typically bi-endinan (or fixed to little) and with Xbox having moved to x64 I don't think anyone cares about PowerPC anymore. Still, did just come across some PAL code with if# BIGENDIANso I wonder if there's anything I should check.

[karelz]

How is endianness related to CheckForOverflowUnderflow? It should be transparent to most of the system, right?

[ghost]

It is not, but keep in mind that I'm working my way through overflow exception; I'm not doing a manual code inspection so any code that's big-endian only will go untested with the current approach.

[karelz]

I see.
We should make at least small effort to not change code which we won't cover in tests. That said, I expect we will make a few mistakes -- CI will catch such cases. If particular build is not covered by CI, I am fine discovering such issues later.

[ghost]

Fair enough. That said, what about the System.IO.Ports tests? Since I'm running the tests in a VM that doesn't even have a (virtual) serial port I get 425 skipped tests. Is there a virtual null modem I could install to get those to run?

[ghost]

Update: I have an almost clean inner loop run on Windows 10 at this point. Outer loop runs and inner loop on macOS and Ubuntu are not clean but as far as I can tell the failures are all due to environmental issues (e.g. DNS tests, HTTP tests, certificate tests). Regarding the last inner loop issue on Windows 10 I've already enlisted @tarekgh's support (thanks Tarek! :)) as I'm not sure what's going on.

[tarekgh]

People on this issue, we are getting close to enable CheckForOverflowUnderflow for the whole corefx for the debug build. this may happen in the next couple of days. please let's know if you have any concern before doing that. Thanks.

[ghost]

@tarekgh @karelz I have not created a PR yet but I have prepared a new branch and this one includes the change to dir.props: https://github.com/dennisdietrich/corefx/commits/Unchecked-Part9

Local inner loop results:

• Windows 10: Clean
• macOS 10.12: A few failures due to my network setup (I think)
• Ubuntu 16.04: Clean (one failure, but it's a known issue)

However, I can't get a clean outer loop run on any of my VMs but going through the test failures they still look like environmental issues. That said, at this point I would like to get test run results from the CI machines.
Also, I'd like to point out that there was at least one test (i.e. I only noticed and confirmed it in one case) that didn't consistently fail because of how the scenario used random data, so there may be issues left even in inner loop runs that I just haven't hit.

[tarekgh]

@dennisdietrich you may start submitting the PR and enable all CI checks on this PR and let's look what is failing there and we can progress from there.

[tarekgh]

@dennisdietrich I would to thank you for all your work done here. that is really awesome.

[ghost]

Don't mention it. Happy to contribute. :)

[karelz]

Agreed, great job @dennisdietrich.
If you're interested in more CoreFX contributions, would recommend to grab some 'up for grabs' issues (which are not API proposals) - the most valuable/impactful things are:

1. 2.0.0 (next release) up for grabs issues
2. wishlist issues
3. simple issues for newbie contributors (not applied too much yet - we just started experimenting with it recently)

[ghost]

@karelz Sure am. Looks like dotnet/corefx#14344 will be next.